2018200 - Wrong len passed to tls_SignOrVerifyUpdate for kCtxStr

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Maurice Dauer [:mdauer]]

Currently, the pointer of kCtxStr is advanced before calling strlen (https://searchfox.org/nss/rev/32c217f646beddadebfd7fb6328ddfc5d37c7ae3/lib/ssl/tls13subcerts.c#325):

    rv = tls_SignOrVerifyUpdate(ctx, kCtxStr,
                                strlen((const char *)kCtxStr + 1 /* 0-byte */));

Instead of:

    rv = tls_SignOrVerifyUpdate(ctx, kCtxStr,
                                strlen((const char *)kCtxStr) + 1 /* 0-byte */);

Comment 1

[Maurice Dauer [:mdauer]]

I don't think this has any security implications, but marking this security-sensitive out of precaution. Bob, what do you think?

Comment 2

[Maurice Dauer [:mdauer]]

Attachment: Bug 2018200 - Fix kCtxStr len passed to tls_SignOrVerifyUpdate, r?rrelyea,#nss-reviewers

Comment 3

[Anna Weine]

It took me 5 minutes to find the difference...

Logically,

rv = tls_SignOrVerifyUpdate(ctx, kCtxStr,
                            strlen((const char *)kCtxStr) + 1 /* 0-byte */);

seems to be correct, we feed to the sign function the kCtxStr and ask to use lengths of kCtxStr bytes + the \0.

But then:
imagine the kCtxStr is "a" "b" "c". In memory it is "a" "b" "c" \0.
in the right case, strLen(kCtxStr) + 1 gives us 4?
in the wrong case, strlen(kCtxStr + 1) = 2, right?

Such way, in the right case we gonna SignOnVerify("abc0"), otherwise SignOnVerify("ab")?

Comment 4

[Robert Relyea]

yeah, this isn't a security issue, It's a correctness issue. We can clear the security issue from the bug.

Comment 5

[Robert Relyea]

I verfied the downstream code was correct. The change was made after a review comment on https://phabricator.services.mozilla.com/D263840 (sigh).

Comment 6

[Robert Relyea]

I've added Standa unto this bug. He says that currently we don't do delegated certificates in our interop testing, which is why it wasn't caught :(

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/nss/rev/1388dbf858b3
Fix kCtxStr len passed to tls_SignOrVerifyUpdate, r=rrelyea,nss-reviewers,keeler