Fix IPv6 netmask parsing for prefixes that end mid-word - #167
Merged
Conversation
TGeneralName.ParseIPv6Mask built the trailing partial word as $FFFF shr (16 - LBits), which sets the low bits rather than the leading ones. Any prefix length that is not a multiple of 16 therefore produced an inverted mask word: /47 encoded 0x7FFF where 0xFFFE was required, and /49 encoded 0x0001 instead of 0x8000. The full words ahead of it were already correct, so the resulting iPAddress GeneralName is wrong only in the boundary word — quiet enough to survive unnoticed. It survived because every existing case used /48, /128 or a literal colon-form mask, all of which either skip the partial-word branch or bypass it entirely. Shift the other way and invert instead, and add /47 and /49 cases so the branch is actually exercised. Also in this change: - TCryptoLibConfig.X509.AllowEmptyIssuerCert: RFC 5280 sec. 4.1.2.4 requires a non-empty issuer DN and the parser rejects an empty one, but profiles that use a self-signed certificate purely as an identity carrier place no requirement on the field, and such certificates are in circulation. The switch is off by default and read-side only — it gates the TTbsCertificateStructure parse constructor alone; the public constructor and both TBS generators stay unconditional. The new test asserts the parse round-trips the original DER byte-identically and re-runs the three rejection paths with the switch on to prove generation is still strict. - IAsn1TaggedObject.HasTagNo, for symmetry with the existing HasTag / HasTagClass / HasContextTag trio. No consumers yet. - Regression test for Pkcs12Store.GetCertificateAlias: 16 certificate entries, each asserted to map back to its own alias before and after a save/reload, plus an absent certificate yielding no alias. The lookup walks key and value together so it was never at risk of pairing them positionally; the test locks that in. - Rename the private TBigInteger.GetLowestSetBitMaskFirst parameter AFirstWordMaskX to AFirstWordMask. Cosmetic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TGeneralName.ParseIPv6Mask built the trailing partial word as $FFFF shr (16 - LBits), which sets the low bits rather than the leading ones. Any prefix length that is not a multiple of 16 therefore produced an inverted mask word: /47 encoded 0x7FFF where 0xFFFE was required, and /49 encoded 0x0001 instead of 0x8000. The full words ahead of it were already correct, so the resulting iPAddress GeneralName is wrong only in the boundary word — quiet enough to survive unnoticed.
It survived because every existing case used /48, /128 or a literal colon-form mask, all of which either skip the partial-word branch or bypass it entirely. Shift the other way and invert instead, and add /47 and /49 cases so the branch is actually exercised.
Also in this change:
TCryptoLibConfig.X509.AllowEmptyIssuerCert: RFC 5280 sec. 4.1.2.4 requires a non-empty issuer DN and the parser rejects an empty one, but profiles that use a self-signed certificate purely as an identity carrier place no requirement on the field, and such certificates are in circulation. The switch is off by default and read-side only — it gates the TTbsCertificateStructure parse constructor alone; the public constructor and both TBS generators stay unconditional. The new test asserts the parse round-trips the original DER byte-identically and re-runs the three rejection paths with the switch on to prove generation is still strict.
IAsn1TaggedObject.HasTagNo, for symmetry with the existing HasTag / HasTagClass / HasContextTag trio. No consumers yet.
Regression test for Pkcs12Store.GetCertificateAlias: 16 certificate entries, each asserted to map back to its own alias before and after a save/reload, plus an absent certificate yielding no alias. The lookup walks key and value together so it was never at risk of pairing them positionally; the test locks that in.
Rename the private TBigInteger.GetLowestSetBitMaskFirst parameter AFirstWordMaskX to AFirstWordMask. Cosmetic.