8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint

[wangweij]

Print out "no limit" instead. This is the words RFC 5280 uses: "Where pathLenConstraint does not appear, no limit is imposed".

No regression test. Trivial.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint

Reviewers

• Jamil Nimeh (@jnimeh - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2493/head:pull/2493
$ git checkout pull/2493

[bridgekeeper]

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@wangweij The following label will be automatically applied to this pull request:

• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (ddb9197)

[jnimeh]

Looks fine to me.

[openjdk]

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint

Reviewed-by: jnimeh

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 95 new commits pushed to the master branch:

• 699a3cd: 8223188: Removed unnecessary #ifdef __cplusplus from .cpp sources
• 05c6009: 8259656: fixpath.sh changes broke _NT_SYMBOL_PATH in RunTests.gmk
• ef7ee3f: 8225081: Remove Telia Company CA certificate expiring in April 2021
• 7c565f8: 8261209: isStandalone property: remove dependency on pretty-print
• 01d9280: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
• a00b130: 8261356: Clean up enum G1Mark
• becee64: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
• f395ee0: 8261306: ServiceLoader documentation has malformed Unicode escape
• 8f4c15f: 8198540: Dynalink leaks memory when generating type converters
• edd5fc8: 8261096: Convert jlink tool to use Stream.toList()
• ... and 85 more: https://git.openjdk.java.net/jdk/compare/f025bc1d5d81532a3bdb87665537de4aaf15b7ea...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[wangweij]

/integrate

[openjdk]

@wangweij Since your change was applied there have been 95 commits pushed to the master branch:

• 699a3cd: 8223188: Removed unnecessary #ifdef __cplusplus from .cpp sources
• 05c6009: 8259656: fixpath.sh changes broke _NT_SYMBOL_PATH in RunTests.gmk
• ef7ee3f: 8225081: Remove Telia Company CA certificate expiring in April 2021
• 7c565f8: 8261209: isStandalone property: remove dependency on pretty-print
• 01d9280: 8261299: Use-after-free on failure path in LinuxPackage.c, getJvmLauncherLibPath
• a00b130: 8261356: Clean up enum G1Mark
• becee64: 8261279: sun/util/resources/cldr/TimeZoneNamesTest.java timed out
• f395ee0: 8261306: ServiceLoader documentation has malformed Unicode escape
• 8f4c15f: 8198540: Dynalink leaks memory when generating type converters
• edd5fc8: 8261096: Convert jlink tool to use Stream.toList()
• ... and 85 more: https://git.openjdk.java.net/jdk/compare/f025bc1d5d81532a3bdb87665537de4aaf15b7ea...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4619f37.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[mlbridge]

Mailing list message from Michael StJohns on security-dev:

On 2/9/2021 9:02 PM, Weijun Wang wrote:

Sorry - not quite right, it's not quite that trivial a fix.

The definition for BasicConstraints is

BasicConstraints ::= SEQUENCE {
cA BOOLEAN DEFAULT FALSE,
pathLenConstraint INTEGER (0..MAX) OPTIONAL }

If pathLenConstraint is not present, then the path length is infinite.??
The flag value for that looks like it was encoded as both any negative
number (and set to -1 to start) and Integer.MAX_VALUE.? I'm not quite
sure why it was done that way, but there's a problem doing that -
actually a bunch of them.

You really ought to get the same encoding coming and going (e.g.
creating an object from DER should encode back to the exact same DER).?
The current code does not do that.

1) It's not valid to encode or decode pathLenConstraint in the DER as a
negative number.?? This isn't a problem for encoding, but the
BasicConstraintsException(Boolean critical, Object value) needs a value
check after line 157 to make sure that the decoded pathLengthConstraint
field is positive - i'd throw an IOException on failure.??? I'd also
change line 149 to just return - the initial value of pathLen is -1 and
that's an appropriate flag for a missing field.

2) I'm not sure why the set/get methods were added.? I think it was to
provide access for the PathValidation stuff? Given that they are
present, I'd insert a line after line 222 (set) : "if (pathLen < 0)
pathLen = -1;" // treat any negative value as unconstrained path length

3) And since the only place pathLen is available externally is in the
get method, I'd change line 237 to "return (pathLen < 0) ?
Integer.MAX_VALUE : Integer.valueOf(pathLen);"?? I think this is more
correct than returning -1.

4) And to fix the problem that led to this discussion, change line 176
to 'pathLenAsString = " unconstrained"' and delete lines 177-178.? E.g.
there's no such thing as undefined here - both a negative number and
MAX_VALUE mean unconstrained length in the previous version of the code.

5) One more - in the other constructor, change line 108 to "this.pathLen
= (len < 0 || len == Integer.MAX_VALUE) ? -1 : len;"

6) *sigh* Delete lines 197-201.? I have no idea why they are overriding
the specified value of critical based on whether ca is true or not, but
it's wrong.??? Conversely, the call to the constructor at line 95 is
correct.

Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210210/b9d786bf/attachment-0001.htm>

[wangweij]

Sorry - not quite right, it's not quite that trivial a fix.

Thanks for all the points. I've filed https://bugs.openjdk.java.net/browse/JDK-8261513, which will be non-trivial.

[mlbridge]

Mailing list message from Sean Mullan on security-dev:

Michael,

Thanks for the comments - a couple of observations of mine below --

On 2/10/21 12:53 AM, Michael StJohns wrote:

On 2/9/2021 9:02 PM, Weijun Wang wrote:

Sorry - not quite right, it's not quite that trivial a fix.

The definition for BasicConstraints is

BasicConstraints ::= SEQUENCE {
cA BOOLEAN DEFAULT FALSE,
pathLenConstraint INTEGER (0..MAX) OPTIONAL }

If pathLenConstraint is not present, then the path length is infinite.
The flag value for that looks like it was encoded as both any negative
number (and set to -1 to start) and Integer.MAX_VALUE.? I'm not quite
sure why it was done that way, but there's a problem doing that -
actually a bunch of them.

You really ought to get the same encoding coming and going (e.g.
creating an object from DER should encode back to the exact same DER).
The current code does not do that.

1) It's not valid to encode or decode pathLenConstraint in the DER as a
negative number.?? This isn't a problem for encoding, but the
BasicConstraintsException(Boolean critical, Object value) needs a value
check after line 157 to make sure that the decoded pathLengthConstraint
field is positive - i'd throw an IOException on failure.

I agree.

??? I'd also
change line 149 to just return - the initial value of pathLen is -1 and
that's an appropriate flag for a missing field.

Some of this is due to the API for
X509Certificate.getBasicConstraints(), and this code tries to return
values that comply with that API. That API returns an integer which is
specified as:

"the value of pathLenConstraint if the BasicConstraints extension is
present in the certificate and the subject of the certificate is a CA,
otherwise -1. If the subject of the certificate is a CA and
pathLenConstraint does not appear, Integer.MAX_VALUE is returned to
indicate that there is no limit to the allowed length of the
certification path."

We need to be careful that for any changes we make, we still comply with
the API.

2) I'm not sure why the set/get methods were added.? I think it was to
provide access for the PathValidation stuff? Given that they are
present, I'd insert a line after line 222 (set) : "if (pathLen < 0)
pathLen = -1;" // treat any negative value as unconstrained path length

In this case, I would throw an Exception. If the pathLenConstraint is
unconstrained, it should not call this method at all.

The internal X509 classes have set/get/delete methods dating back to the
original design.

3) And since the only place pathLen is available externally is in the
get method, I'd change line 237 to "return (pathLen < 0) ?
Integer.MAX_VALUE : Integer.valueOf(pathLen);"?? I think this is more
correct than returning -1.

I think it is better to ensure that in this class, pathLen is never < 0,
and maybe create an extra boolean flag to indicate if there is a
pathLenConstraint field, or alternatively use an Integer object where
null means there is no field. I think a lot of these issues are due to
the fact that in this class -1 can mean either a bad value, or an
unconstrained length.

4) And to fix the problem that led to this discussion, change line 176
to 'pathLenAsString = " unconstrained"' and delete lines 177-178.? E.g.
there's no such thing as undefined here - both a negative number and
MAX_VALUE mean unconstrained length in the previous version of the code.

5) One more - in the other constructor, change line 108 to "this.pathLen
= (len < 0 || len == Integer.MAX_VALUE) ? -1 : len;"

6) *sigh* Delete lines 197-201.? I have no idea why they are overriding
the specified value of critical based on whether ca is true or not, but
it's wrong.??? Conversely, the call to the constructor at line 95 is
correct.

That does seem a little weird.

--Sean