1275309 - replace nsSecretDecoderRing::encode and ::decode with a call to Base64Encode and Base64Decode each

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

nsSecretDecoderRing::encode and ::decode are private to nsSecretDecoderRing and are only called once each. Furthermore, they should really only be one line (modulo argument checks, etc.): Base64Encode and Base64Decode, as appropriate. We should just remove them and call Base64Encode/Base64Decode (as a bonus, this will potentially address bug 953057, or at least stop the crashes).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add a variant of Base64Decode()

This mirrors the changes made to encoding in bug 1273712.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use Base64Decode() in nsSecretDecoderRing::decode()

I ended up keeping nsSecretDecoderRing::{encode,decode} because there's enough
argument checking and fiddling that I prefer to have it in its own functions.

Comment 3

[:Cykesiopka]

Comment on attachment 8756183 [details] [diff] [review]
(part 2) - Use Base64Decode() in nsSecretDecoderRing::decode()

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

Thanks for working on this.
The change in general is sound, but there are some issues that need to be addressed.

::: security/manager/ssl/nsSDR.cpp
@@ +311,5 @@
>  nsSecretDecoderRing::decode(const char* data, unsigned char** result,
>                              int32_t* _retval)
>  {
> +  uint32_t dataLen = strlen(data);
> +  char* binary;

Let's explicitly initialise this to nullptr - it's preferable to not have to deal with uninitialised memory.

@@ +320,3 @@
>    }
> +  *result = BitwiseCast<unsigned char*>(binary);
> +  *_retval = AssertedCast<int32_t>(binaryLen);

"mozilla/Casting.h" should be included for these. If you feel like it, please sort the includes as well.

@@ +320,4 @@
>    }
> +  *result = BitwiseCast<unsigned char*>(binary);
> +  *_retval = AssertedCast<int32_t>(binaryLen);
> +  return rv;

This delayed return is problematic when combined with the uninitialised |binary| pointer above:

./mach xpcshell-test security/manager/ssl/tests/unit/test_sdr.js
[...]
> "/home/moz/mozilla-inbound/memory/mozjemalloc/jemalloc.c:3228: Failed assertion: "rbp_i_cmp > 0""
> "Hit MOZ_CRASH() at /home/moz/mozilla-inbound/memory/build/jemalloc_config.cpp:163"

Let's have an NS_FAILED(rv) check immediately after the Base64Decode() decode call - if calls like these fail, PSM style is to assume we can't rely on the outparams being valid.

Comment 4

[:Cykesiopka]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Created attachment 8756183 [details] [diff] [review]
> (part 2) - Use Base64Decode() in nsSecretDecoderRing::decode()
> 
> I ended up keeping nsSecretDecoderRing::{encode,decode} because there's
> enough
> argument checking and fiddling that I prefer to have it in its own functions.

I believe all this argument checking and casting can be removed if we refactor the IDL and the implementation, but it's up to you if you want to work on that as well.

If not, we can rescope this bug, and I'll file a follow up later for the refactoring stuff.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use Base64Decode() in nsSecretDecoderRing::decode()

With comments addressed.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use unsigned integers for lengths in nsSecretDecoderRing

This is a clear improvement. Is changing the buffers from |unsigned char*| to
|char*| ok? Because that would improve things further.

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Is changing the buffers from |unsigned char*| to
> |char*| ok? Because that would improve things further.

Actually, that's looks difficult because NSS uses |unsigned char*| for various things.

Comment 8

[:Cykesiopka]

Comment on attachment 8756699 [details] [diff] [review]
(part 2) - Use Base64Decode() in nsSecretDecoderRing::decode()

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

Looks good.

::: security/manager/ssl/nsSDR.cpp
@@ +314,5 @@
>  {
> +  uint32_t dataLen = strlen(data);
> +  char* binary = nullptr;
> +  uint32_t binaryLen = 0;
> +  nsresult rv = Base64Decode(data, dataLen, &binary, &binaryLen);

IIUC, this change means this line in DecryptString():
> if (decoded) PR_DELETE(decoded);
should use free() now.

(I wish we didn't have so many nominally different memory allocators...)

@@ +321,2 @@
>    }
> +  if (NS_FAILED(rv)) {

This should normally be before the |binaryLen| check, but I see the |binaryLen| check gets removed in part 3, so OK.

Comment 9

[:Cykesiopka]

Comment on attachment 8756700 [details] [diff] [review]
(part 3) - Use unsigned integers for lengths in nsSecretDecoderRing

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

Yes, this is indeed nicer.

::: security/manager/ssl/nsSDR.cpp
@@ +297,5 @@
>  
>  // Support routines
>  
>  nsresult
> +nsSecretDecoderRing::encode(const unsigned char* data, uint32_t dataLen,

Let's inline this function into the caller.

@@ +305,4 @@
>  }
>  
>  nsresult
>  nsSecretDecoderRing::decode(const char* data, unsigned char** result,

This function is simple enough now that IMO it's not that bad to inline it as well.

Comment 10

[:Cykesiopka]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> (In reply to Nicholas Nethercote [:njn] from comment #6)
> > Is changing the buffers from |unsigned char*| to
> > |char*| ok? Because that would improve things further.
> 
> Actually, that's looks difficult because NSS uses |unsigned char*| for
> various things.

Yeah, doing this would probably just result in moving the casts around.

Comment 11

[:Cykesiopka]

I filed Bug 1275841 for future clean ups.

Comment 12

[Nicholas Nethercote [inactive]]

> IIUC, this change means this line in DecryptString():
> > if (decoded) PR_DELETE(decoded);
> should use free() now.

Good catch. In fact, thank you for all your good catches to my sloppy mistakes in this bug and the previous one.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Inline nsSecretDecoderRing::{decode,encode}

Much better.

Comment 14

[:Cykesiopka]

Comment on attachment 8757105 [details] [diff] [review]
(part 4) - Inline nsSecretDecoderRing::{decode,encode}

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

Cool.

Comment 15

[:Cykesiopka]

Comment on attachment 8756181 [details] [diff] [review]
(part 1) - Add a variant of Base64Decode()

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

::: xpcom/io/Base64.cpp
@@ +390,5 @@
> +
> +  uint32_t binaryLen = (aBase64Len * 3) / 4;
> +
> +  // Add one byte for null termination.
> +  char* binary = (char*)malloc(binaryLen + 1);

FWIW, it looks like you can include "mozilla/UniquePtrExtensions.h" and change this line to something like
> UniqueFreePtr<char[]> binary(static_cast<char*>(malloc(binaryLen + 1)));
... so there's less manual memory management.

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8756181 [details] [diff] [review]
(part 1) - Add a variant of Base64Decode()

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

Looks good, some thoughts on code duplication, buffer termination, and the null buffer case.

::: xpcom/io/Base64.cpp
@@ +382,5 @@
> +  // Don't ask PR_Base64Decode to decode the empty string.
> +  if (aBase64Len == 0) {
> +    char* binary = (char*)moz_xmalloc(1);
> +    binary[0] = '\0';
> +    *aBinary = binary;

Why are we allocating at all? Can't we just set the buffer to null?

@@ +390,5 @@
> +
> +  uint32_t binaryLen = (aBase64Len * 3) / 4;
> +
> +  // Add one byte for null termination.
> +  char* binary = (char*)malloc(binaryLen + 1);

|binary| is used for the out param, by design I don't think there's a way to steal the thing the UniquePtr is pointing to.

@@ +395,5 @@
> +  if (!binary) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  if (!PL_Base64Decode(aBase64, aBase64Len, binary)) {

Can we refactor this logic so it's shared by the char* and nsACString versions? So from here to terminating the binary.

Something like |bool DecodeIntoBuffer(base64, len, binary)|.

@@ +410,5 @@
> +    } else {
> +      binaryLen -= 1;
> +    }
> +  }
> +  binary[binaryLen] = '\0';

With a raw buffer why are we even terminating the buffer? If we don't need it we should also adjust the malloc call above.

@@ +440,3 @@
>    }
>  
> +  char* binary = aBinary.BeginWriting();

IIRC BeginWriting is infallible, checking |binary| below should be unnecessary.

Comment 17

[:Cykesiopka]

(In reply to Eric Rahm [:erahm] from comment #16)
> @@ +390,5 @@
> > +
> > +  uint32_t binaryLen = (aBase64Len * 3) / 4;
> > +
> > +  // Add one byte for null termination.
> > +  char* binary = (char*)malloc(binaryLen + 1);
> 
> |binary| is used for the out param, by design I don't think there's a way to
> steal the thing the UniquePtr is pointing to.

UniqueFreePtr binary(...);
[...]
*aBinary = binary.release();
... should work.

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

How is this going? Is there anything I can do to assist in completing this?

Comment 19

[Nicholas Nethercote [inactive]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #18)
> How is this going? Is there anything I can do to assist in completing this?

Thank you for the offer. I just need to get off my lazy butt and address the review comments. I should get to it in the next day or two.

Comment 20

[Nicholas Nethercote [inactive]]

> ::: xpcom/io/Base64.cpp
> @@ +382,5 @@
> > +  // Don't ask PR_Base64Decode to decode the empty string.
> > +  if (aBase64Len == 0) {
> > +    char* binary = (char*)moz_xmalloc(1);
> > +    binary[0] = '\0';
> > +    *aBinary = binary;
> 
> Why are we allocating at all? Can't we just set the buffer to null?

Not sure. I'd rather err on the side of safety for now.


> > +  uint32_t binaryLen = (aBase64Len * 3) / 4;
> > +
> > +  // Add one byte for null termination.
> > +  char* binary = (char*)malloc(binaryLen + 1);
> 
> |binary| is used for the out param, by design I don't think there's a way to
> steal the thing the UniquePtr is pointing to.

There is a way: release(). I've used UniqueFreePtr as Cykesiopka suggested, and used release() with it.

 
> @@ +395,5 @@
> > +  if (!binary) {
> > +    return NS_ERROR_OUT_OF_MEMORY;
> > +  }
> > +
> > +  if (!PL_Base64Decode(aBase64, aBase64Len, binary)) {
> 
> Can we refactor this logic so it's shared by the char* and nsACString
> versions? So from here to terminating the binary.
> 
> Something like |bool DecodeIntoBuffer(base64, len, binary)|.

Done.


> @@ +410,5 @@
> > +    } else {
> > +      binaryLen -= 1;
> > +    }
> > +  }
> > +  binary[binaryLen] = '\0';
> 
> With a raw buffer why are we even terminating the buffer? If we don't need
> it we should also adjust the malloc call above.

That's the API I've chosen -- it's a C string, i.e. null-terminated. Seems safer than a non-null-terminated char buffer for a success case. The failure case uses nullptr.


> @@ +440,3 @@
> >    }
> >  
> > +  char* binary = aBinary.BeginWriting();
> 
> IIRC BeginWriting is infallible, checking |binary| below should be
> unnecessary.

Good catch. The SetCapacity() above it is fallible, so we can use the default BeginWriting() here, which is infallible.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add a variant of Base64Decode()

This mirrors the changes made to encoding in bug 1273712.

Comment 22

[Eric Rahm [:erahm]]

Comment on attachment 8777631 [details] [diff] [review]
(part 1) - Add a variant of Base64Decode()

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

::: xpcom/io/Base64.cpp
@@ +369,5 @@
>    return rv;
>  }
>  
> +static nsresult
> +Base64DecodeHelper(const char* aBase64, uint32_t aBase64Len, char* aBinary,

Helper's not super descriptive, perhaps |DecodeAndTerminate| instead? It's not a huge deal as this is internal.

Comment 23

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/746392907239ebc994cefb5bd0aac5d1b92171ac
Bug 1275309 (part 1) - Add a variant of Base64Decode(). r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff94bac1490f9609fb6a954792db236c1f8e2fe
Bug 1275309 (part 2) - Use Base64Decode() in nsSecretDecoderRing::decode(). r=cykesiopka.

https://hg.mozilla.org/integration/mozilla-inbound/rev/03d49e82f8e2fc85de2b44ab827bff186d8cc3f1
Bug 1275309 (part 3) - Use unsigned integers for lengths in nsSecretDecoderRing. r=cykesiopka.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d72c6106ad328e740f0223b1f53cce799442f2b
Bug 1275309 (part 4) - Inline nsSecretDecoderRing::{decode,encode}. r=cykesiopka.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/746392907239
https://hg.mozilla.org/mozilla-central/rev/5ff94bac1490
https://hg.mozilla.org/mozilla-central/rev/03d49e82f8e2
https://hg.mozilla.org/mozilla-central/rev/1d72c6106ad3