Support ASN.1 tag encoding with values greater than 30. #2066
Conversation
There was a problem hiding this comment.
Why are you using BigInteger? $child['constant'] is an integer and converting that to a BigInteger at the point where you are isn't going to all of a sudden enable you to use bigger integers. And although there's absolutely a use case for being able to do constants > 30 is there one for being able to do constants > PHP_INT_MAX?
Other than that I didn't realize that this wasn't supported. Like phpseclib can decode ASN.1 stuff with > 30 but it can't encode them. So nice catch there!
A unit test would be good, as well.
If it helps (and partially for my own reference lol) this is what I used to test it:
$map = [
'type' => ASN1::TYPE_SEQUENCE,
'children' => [
'demo' => [
'constant' => 0xFFFFFFFF,
'optional' => true,
'explicit' => true,
'default' => 'v1',
'type' => ASN1::TYPE_INTEGER,
'mapping' => ['v1', 'v2', 'v3']
]
]
];
$data = ['demo' => 'v3']; // should be 300abf8fffffff7f03020102
$str = ASN1::encodeDER($data, $map);
$decoded = ASN1::decodeBER($str);
$arr = ASN1::asn1map($decoded[0], $map);
phpseclib/File/ASN1.php
Outdated
| $subtagvalue = $constant->bitwise_and($mask); | ||
| $subtagvalue->setPrecision(8); | ||
| $subtag = (chr(0x80) | $subtagvalue->toBytes()) . $subtag; | ||
| $constant = $constant->bitwise_rightShift(shift: 7); |
There was a problem hiding this comment.
Using named arguments for a function that only has one argument seems unnecessary. And inconsistent. Like if you're going to do it for this method then why not do it for $subtagvalue->setPrecision(8) as well? Not that I'm saying you ought to.
- Fix inconsistent named arguments usage - Add Unit test of ASN1 DER encoding with constants > 30
BigInteger was used for coherence with the implementation of object identifier values File/ASN1.php line 1232. If it's not relevant, the implementation can be done without using BigInteger. I'm not sure if there's a use case with constants > PHP_INT_MAX, since PHP_INT_MAX is quite big. Other requested changes were done in the last commit. |
Good catch - that's prob something that ought to be changed. I can do that. That said, altho encoding with tags using BigInteger is consistent with how phpseclib handles OIDs, it's inconsistent with how phpsecib decodes tags: phpseclib/phpseclib/File/ASN1.php Lines 234 to 253 in baba459 So if you could change it to use ints that'd be great. Meanwhile, I'll update how OIDs are handled. Thanks!! |
So I modified ASN1.php to make it stop using BigIntegers for OIDs and one of the unit tests then failed. Turns out phpseclib started using BigInteger's because of #1367. So I guess there is a valid use-case for using BigInteger's for OIDs but I am aware of no valid use case for using them with tags. And plus, as previously stated, BigInteger's are not used for decoding tags. Anyway, imma try to squash your commits into one and then cherry-pick that to the 3.0 branch. Maybe this evening or tomorrow or some such. Thanks! |
|
This has been squashed into one commit and cherry-picked to the 3.0 branch: Thanks! |
Allow ASN.1 DER encoding of tags with values greater than 30.
From X.690-0207.pdf#page=12 section 8.1.2.4: