1431940 - BLAKE2B_Update may dereference null pointer

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[jbonnafo]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

First line of code of function BLAKE2B_Update uses ctx pointer.
However there is no check on this pointer being not null.


Actual results:

There is a check later in the code (line 232), it may be good to check at the beginning of function, before trying to use the ctx variable.


Expected results:

There is a check later in the code (line 232), it may be good to check at the beginning of function, before trying to use the ctx variable.

Comment 1

[Venkatesh (:vpit3833)]

I like to work on this bug.  Is the below diff good?

diff --git a/security/nss/lib/freebl/blake2b.c b/security/nss/lib/freebl/blake2b.c
--- a/security/nss/lib/freebl/blake2b.c
+++ b/security/nss/lib/freebl/blake2b.c
@@ -218,6 +218,7 @@ SECStatus
 BLAKE2B_Update(BLAKE2BContext* ctx, const unsigned char* in,
                unsigned int inlen)
 {
+    PORT_Assert(ctx != NULL);
     size_t left = ctx->buflen;
     size_t fill = BLAKE2B_BLOCK_LENGTH - left;

Comment 2

[Franziskus Kiefer [:franziskus]]

The assert is only active in debug builds. So an if(!ctx) is needed as well. Can you please upload the patch to phabricator (https://phabricator.services.mozilla.com/) or here in splinter?

Comment 3

[Venkatesh (:vpit3833)]

Attachment: bug-1431940-attempt-01.patch

What is splinter?

Please comment on this patch.

Comment 4

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8966904 [details] [diff] [review]
bug-1431940-attempt-01.patch

Review of attachment 8966904 [details] [diff] [review]:
-----------------------------------------------------------------

This is splinter :)
Can you make the patch against NSS? This is against Firefox.

::: security/nss/lib/freebl/blake2b.c
@@ +218,5 @@
>  BLAKE2B_Update(BLAKE2BContext* ctx, const unsigned char* in,
>                 unsigned int inlen)
>  {
> +    PORT_Assert(ctx != NULL);
> +    if (!ctx) {

We actually check this already further down. But we get `left = ctx->buflen` before that check. So either move the `left` assignment after the check or remove the check further down.

Comment 5

[Venkatesh (:vpit3833)]

Attachment: bug-1431940-attempt-02.patch

Is this better?

Comment 6

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8967230 [details] [diff] [review]
bug-1431940-attempt-02.patch

Review of attachment 8967230 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 7

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/4626e0cede57956b9dbcb72135753668c819a3c0

Comment 8

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/e32e2755932b

This doesn't pass tests. Can you check why this is failing?

Comment 9

[Venkatesh (:vpit3833)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #8)
> https://hg.mozilla.org/projects/nss/rev/e32e2755932b
> 
> This doesn't pass tests. Can you check why this is failing?

Sure, I got the nss.1/results.html by running all.sh, and can see the failed cases.  How do I work backwards from here to the source that is causing the failure?

Comment 10

[Franziskus Kiefer [:franziskus]]

You can see [  FAILED  ] Blake2BTests.NullTest. So you can look at that test (https://searchfox.org/nss/rev/6383002966ffb398fc6a186ad21673ca3f2c79a9/gtests/freebl_gtest/blake2b_unittest.cc#135). You can also run only that test by running freebl_gtest --gtest_filter="*Blake2B*NullTest*". From there you can debug the issue.

Comment 11

[Venkatesh (:vpit3833)]

Attachment: bug-1431940-attempt-03.patch

Preserving the order here.  inlen == 0 followed by ctx and in.  HOST=localhost DOMSUF=localdomain BUILD_OPT=1 USE_64=1 tests/all.sh shows the same output before and after with this patch compiled.  I did not find freebl_test in the nss directory.  Is it the tests under ssl, ssl_gtests, gtests?

Comment 12

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8968725 [details] [diff] [review]
bug-1431940-attempt-03.patch

Review of attachment 8968725 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the two assert moved before the if.

::: lib/freebl/blake2b.c
@@ +231,5 @@
> +    size_t left = ctx->buflen;
> +    size_t fill = BLAKE2B_BLOCK_LENGTH - left;
> +
> +    PORT_Assert(ctx != NULL);
> +    PORT_Assert(in != NULL);

These two asserts only make sense if they're before the if block checking them. Otherwise we'll never get here.

Comment 13

[Dmitry Baryshkov]

Attachment: Bug 1431940 - remove dereference before NULL check in BLAKE2B code.

Comment 14

[Kevin Jacobs [:kjacobs]]

https://hg.mozilla.org/projects/nss/rev/5e661906698fa90702c31eae492c31a080882689