card-rtecp: Fix rtecp_change_reference_data (resolves #931) #958
Conversation
|
The comment in line 450 to 453 is hard to read, and not clear if this is code or not. Can you use: You comment that the card in one case does not support sc_asn1_put_tag properly, but you also use sc_asn1_put_tag in another place. The card may be expecting SIMPLE-TLV not BER-TLV a form of ASN.1. abf8f36 has a version of simpletlv.c and simpletlv.c but its is not in master. Does the card really support chaining (SC_APDU_FLAGS_CHAINING)? If it did, I would not expect each each segment to have to start with the A5 Len. What you might be seeing is the OpenSC code sees the chaining bit set, and sends multiple APDUs |
konstantinpersidskiy
force-pushed
the
rtecp_change_reference_data_fix
branch
from
4ad838e
to
9c01c3f
Compare
|
Comment at 450-453 should really be a bit more neat - thanks for noticing TLV-format used in rtecp is not SIMPLE-TLV and not BER-TLV - that's why we have to put A5 tags in a special loop, not with any of implemented in OpenSC functions. But when TLV length is less than 128, the output of asn1_put_tag() matches the expected format, and for putting TLV with small length asn1_put_tag() is used - to fit into general OpenSC codestyle Rtecp supports chaining, but it's a bit 'special' either. The card doesn't ignore the chaining bit - it is needed to ensure the consistency of chaining APDU, but card also expects each transmit not to cut TLVs in parts, that's why it may look a little confusing. |
src/libopensc/card-rtecp.c
Outdated
| * buf_length <= apdu_num * max_transmit_length; | ||
| * 2 + sizeof(rsf_length) + newlen + 2 * apdu_num <= apdu_num * max_transmit_length; | ||
| * apdu_num >= (2 + sizeof(rsf_length) + newlen) / (max_transmit_length -2) | ||
| */ |
There was a problem hiding this comment.
Please clearify the comments. If nobody knows what you're talking about you might as well remove the comments...
src/libopensc/card-rtecp.c
Outdated
| val_length = max_transmit_length - (p - buf) % max_transmit_length - 2; | ||
| else | ||
| val_length = newlen; | ||
| assert(val_length <= newlen); |
There was a problem hiding this comment.
please replace assert with proper error handling.
|
@frankmorgner , thanks for reviewing the code! The assert you mentioned won't be triggered in any situation in this version of file - that is guaranteed by the if-else block (lines 471-474), but it'll help future developers to control violating of input data borders (if he change code between if-else and assert). Also asserts are used evwerywhere in this particular file, so I decided to follow the style and use them either. I also changed the comment to be more informative and clear.
I'm looking forward to it! |
|
Indeed, we could do much better with the error handling, but that shouldn't keep us from starting somewhere. |
Hello
This branch resolves the issue #931 . I inspected some code and probably found out the cause of the trouble. The problem seemed to appear in OpenSC of version 0.14.0 and higher and it was caused by rtecp-dependent code relying on previous implementation of function asn1_put_tag. So when asn1_put_tag was changed to treat long data more accurate, some of rtecp-dependent code became invalid in some cases. I rewrote that rtecp code and now it seems to work properly when it comes to importing long pieces of data (like RSA keys) to rtecp (tested on Rutoken ECP 2.0).
Regards,
Konstantin Persidskiy