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

[Michael StJohns]

On 2/9/2021 9:02 PM, Weijun Wang wrote:
> On Wed, 10 Feb 2021 01:39:15 GMT, Weijun Wang <weijun at openjdk.org> wrote:
>
>> 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.
> This pull request has now been integrated.
>
> Changeset: 4619f372
> Author:    Weijun Wang <weijun at openjdk.org>
> URL:       https://git.openjdk.java.net/jdk/commit/4619f372
> Stats:     11 lines in 1 file changed: 8 ins; 1 del; 2 mod
>
> 8261472: BasicConstraintsExtension::toString shows "PathLen:2147483647" if there is no pathLenConstraint
>
> Reviewed-by: jnimeh
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/2493


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.org/pipermail/security-dev/attachments/20210210/b9d786bf/attachment.htm>