Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Certificate parsing fixes. #476

Merged
merged 13 commits into from
Feb 3, 2017
Merged

Certificate parsing fixes. #476

merged 13 commits into from
Feb 3, 2017

Conversation

davidlehn
Copy link
Member

  • Fixes possible issues with certificate parsing.
  • Eliminates many parsing failures. Often due to parsing BIT STREAMs as possible ASN.1 data.
  • Improve many checks to protect against getting out of sync with byte stream.
  • Add support for "raw" data to ensure BIT STREAM data stays constant while capturing or encoding when it may have mistakenly been interpreted as ASN.1 data.

- The current get/put implementations only support a limited set of bit
  values. Enforce this by checking the param.
- Sort case statement by id value.
- Show hex of data.
- Show "unused" bit count.
@davidlehn davidlehn added this to the v0.7.0 milestone Jan 25, 2017
@davidlehn davidlehn self-assigned this Jan 25, 2017
@davidlehn davidlehn requested a review from dlongley January 25, 2017 00:55
@davidlehn
Copy link
Member Author

Note these fixes also cleanly apply to 0.6.x and a branch is available:
https://github.com/digitalbazaar/forge/tree/cert-fixes-0.6.x

@@ -248,141 +254,277 @@ var _getValueLength = asn1.getBerValueLength = function(b) {
};

/**
* Check the byte buffer has enough bytes. Throws an Error if not.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Check the byte" => "Check if the byte"

tagClass: tagClass,
type: type,
constructed: constructed,
composed: constructed || forge.util.isArray(value),
value: value
};
if(options && 'raw' in options) {
obj.raw = options.raw;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of the name raw. I don't feel like it's descriptive enough for what's going on, but since I don't have a better name, we can go with it. It's not difficult to alias later and deprecate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Bits? Bytes? Octets? Data?

* [strict] true to be strict when checking value lengths, false to
* allow truncated values (default: true).
* [decodeBinaryStrings] true to attempt to decode the content of
* BIT STRINGs (not OCTET STRINGs) in strict mode. Note that without
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this only applies to BIT STRINGS, let's call the option decodeBitStrings. I think our hope is to never support it with OCTET STRINGS because the better implementation would be to use a schema if you want this sort of thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also mention that this flag is a hack that should only be used when absolutely necessary -- and that the schema support mentioned will be a future feature for a robust way to deal with the issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially thought it might control OCTET STRING behavior too, but yes, more direct name now and alternative solutions later sounds good.

}

/**
* Internal functino to parse an asn1 object from a byte buffer in DER format.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

@@ -916,6 +1062,9 @@ asn1.validate = function(obj, v, capture, errors) {
if(v.captureAsn1) {
capture[v.captureAsn1] = obj;
}
if(v.captureRaw && 'raw' in obj) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if captureDer would be a better name. It wouldn't work map cleanly with BER-formatted data, unfortunately.

case asn1.Type.INTEGER:
rval += ' (Integer)';
break;
case asn1.Type.BITSTRING:
rval += ' (Bit string)';
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why move this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to be in the same order as the type ids.

@davidlehn
Copy link
Member Author

Force pushed updates and fixes.
@dlongley: Please review again.

@@ -6,6 +6,9 @@ Forge ChangeLog
### Fixed

- Fix test looping bugs so all tests are run.
- Improved ASN.1 parsing. Many failure cases eliminated. More sanity checks.
Better behavior in default mode of parsing BIT STRINGs. Beter handling of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beter => Better

}

if(typeof obj === 'string') {
return String(obj);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String() cast unnecessary

value: asn1.copy(obj.value, options)
};
if(options && !options.excludeBitStringContents) {
copy.bitStringContents = String(obj.bitStringContents);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String() cast unnecessary. Add a TODO: copy byte buffer if its a buffer not a string for the future.

- equal: deeply check is asn1 objects are equal.
- copy: return a deep copy of an asn1 object.
- Add options object to create() that takes a "bitStringContent" option.
- Stores BIT STRING content in ASN.1 structure.
- Used for BIT STRINGSs to retain the original value even after possible
  decoding.
- toDer() will use the saved contents if present.
- Add validate() "captureBitStringContents" and "captureBitStringValue"
  support to retrieve the saved contents or value without leading unused
  bit counter byte.
- Refactor fromDer algorithm.
- More strictly follow structure lengths.
- Improve remaining data checks to avoid overruns with unchecked buffer get
  calls.
- Improve length sanity checks.
- Allow options for fromDer. Handle old strict call signature.
- Add decodeBitStrings flag to control decoding of BIT STRINGs.
- Improve handling of ASN.1 encapsulated in BIT STRINGs. Many parsing failure
  cases eliminated.
- Store BIT STRING content bytes so toDer() and validate() have access to what
  may have decoded as ASN.1 but was just plain bytes.
- Add tests.

Note that with these changes ASN.1 may still parse BIT STRINGs as ASN.1
even though they are plain bytes. This could happen with signatures.
However, the saved original data is now available and used when going
back to bytes with toDer(). To avoid such parsing completely will
require use of structural schemas.
This helps protect against parsing too large sub structures as valid
data.
- Use BIT STRING capture API to help ensure data is the proper format.
- Avoids the need to check if signatures were parsed as ASN.1.
- Note about future support for arbitrary bit length ids.
@davidlehn
Copy link
Member Author

@dlongley Please review latest commits. Thanks.

@davidlehn davidlehn merged commit e6d0d46 into master Feb 3, 2017
@davidlehn davidlehn deleted the cert-fixes branch February 3, 2017 20:02
@davidlehn davidlehn restored the cert-fixes branch February 3, 2017 20:03
@davidlehn davidlehn deleted the cert-fixes branch February 3, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants