1120937 - TEST-UNEXPECTED-TIMEOUT | security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js | Test timed out

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Mike Hommey [:glandium]]

This happened on linux64 opt builds when turning jemalloc 3 on, but seeing how I fixed it, I wonder how it didn't happen before. Also, it goes away when I turn off free() poisoning.

Essentially, the pkcs11 test module doesn't fill the session number in OpenSession, and that ends up breaking stuff.

Similarly, while they're not involved in this particular breakage, string data fields are not being initialized correctly. When reading them from the end like in PK11_MakeString, which can contain random values, that ends up with garbage copied.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Properly initialize string fields and session field from the PKCS#11 test module

Comment 2

[Mike Hommey [:glandium]]

BTW, valgrind was complaining about all those that I'm fixing here, and doesn't complain anymore with the patch applied.

Comment 3

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8548194 [details] [diff] [review]
Properly initialize string fields and session field from the PKCS#11 test module

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

Thanks for figuring this out!
To be safe, I think we should do a similar thing in Test_C_GetMechanismInfo (should just be able to memset(pInfo, 0, sizeof(CK_MECHANISM_INFO));)
Is the output from the automated valgrind runs available anywhere? It's sad that this information was available yet I was unaware of it for so long...

::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp
@@ +44,5 @@
>    pInfo->cryptokiVersion = CryptokiVersion;
>    static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
>                  "TestManufacturerID too long - make it shorter");
>    memcpy(pInfo->manufacturerID, TestManufacturerID, sizeof(TestManufacturerID));
> +  memset(pInfo->manufacturerID + sizeof(TestManufacturerID), 0,

These memsets all look correct to me, but how about just calling memset(pInfo, 0, sizeof(CK_INFO)); (or CK_SLOT_INFO, etc., as appropriate) once at the top (after the null check)?

@@ +188,5 @@
>    return CKR_FUNCTION_NOT_SUPPORTED;
>  }
>  
>  CK_RV Test_C_OpenSession(CK_SLOT_ID, CK_FLAGS, CK_VOID_PTR, CK_NOTIFY,
> +                         CK_SESSION_HANDLE_PTR session)

I'd prefer this be called 'phSession' to match security/nss/lib/util/pkcs11f.h

Comment 4

[Mike Hommey [:glandium]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Comment on attachment 8548194 [details] [diff] [review]
> Properly initialize string fields and session field from the PKCS#11 test
> module
> 
> Review of attachment 8548194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for figuring this out!
> To be safe, I think we should do a similar thing in Test_C_GetMechanismInfo
> (should just be able to memset(pInfo, 0, sizeof(CK_MECHANISM_INFO));)
> Is the output from the automated valgrind runs available anywhere? It's sad
> that this information was available yet I was unaware of it for so long...

The valgrind output didn't come from automation. I ran valgrind on the xpcshell test that was failed when jemalloc 3 was enabled.

> ::: security/manager/ssl/tests/unit/pkcs11testmodule/pkcs11testmodule.cpp
> @@ +44,5 @@
> >    pInfo->cryptokiVersion = CryptokiVersion;
> >    static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
> >                  "TestManufacturerID too long - make it shorter");
> >    memcpy(pInfo->manufacturerID, TestManufacturerID, sizeof(TestManufacturerID));
> > +  memset(pInfo->manufacturerID + sizeof(TestManufacturerID), 0,
> 
> These memsets all look correct to me, but how about just calling
> memset(pInfo, 0, sizeof(CK_INFO)); (or CK_SLOT_INFO, etc., as appropriate)
> once at the top (after the null check)?

Relatedly, why doesn't the caller do that in NSS (or call calloc to get those buffers)?

Comment 5

[Mike Hommey [:glandium]]

That said, aren't some of these functions like the one for token called with an already filled slot info?

Comment 6

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Mike Hommey [:glandium] from comment #4)
> Relatedly, why doesn't the caller do that in NSS (or call calloc to get
> those buffers)?

calloc isn't used because they're generally stack-allocated. As for why they're not initialized to zero, it looks like this is old, crufty Netscape code that nobody checked for problems like this.

(In reply to Mike Hommey [:glandium] from comment #5)
> That said, aren't some of these functions like the one for token called with
> an already filled slot info?

I had a look around and couldn't find any examples of this. What callsites are you concerned about?

Comment 7

[Mike Hommey [:glandium]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > Relatedly, why doesn't the caller do that in NSS (or call calloc to get
> > those buffers)?
> 
> calloc isn't used because they're generally stack-allocated. As for why
> they're not initialized to zero, it looks like this is old, crufty Netscape
> code that nobody checked for problems like this.

How about fixing that, then?

> (In reply to Mike Hommey [:glandium] from comment #5)
> > That said, aren't some of these functions like the one for token called with
> > an already filled slot info?
> 
> I had a look around and couldn't find any examples of this. What callsites
> are you concerned about?

I haven't looked in detail, but when debugging, it /looked/ like what the token function was getting was already initialized, but I can be wrong.

Comment 8

[Mike Hommey [:glandium]]

Note that PK11_GetTokenInfo does this:

/*
 * some buggy drivers do not fill the buffer completely, 
 * erase the buffer first
 */
PORT_Memset(info->label,' ',sizeof(info->label));
PORT_Memset(info->manufacturerID,' ',sizeof(info->manufacturerID));
PORT_Memset(info->model,' ',sizeof(info->model));
PORT_Memset(info->serialNumber,' ',sizeof(info->serialNumber));

So in fact, the pkcs module is not even supposed to fill with nulls, but with spaces (which matches what PK11_MakeString looks for, actually)

Man, this API is so broken.

Comment 9

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3792557dcf50

Comment 10

[Mike Hommey [:glandium]]

I only landed the session part.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3792557dcf50

Comment 12

[Mike Hommey [:glandium]]

Attachment: Properly initialize string fields from the PKCS#11 test module

The string fields need to be padded with spaces, according to what
PK11_MakeString does to find the end of the string.

While here, factor all the string manipulations in the test module and
use some C++ template magic to do the right thing.

This changes the static asserts from (with clang):

pkcs11testmodule.cpp:45:3: error: static_assert failed
      "TestManufacturerID too long - make it shorter"
  static_assert(sizeof(TestManufacturerID) <= sizeof(pInfo->manufacturerID),
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

to:

pkcs11testmodule.cpp:46:3: error: static_assert failed
      "DestSize >= SrcSize - 1"
  static_assert(DestSize >= SrcSize - 1, "DestSize >= SrcSize - 1");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~
pkcs11testmodule.cpp:58:3: note: in instantiation of function
      template specialization 'CopyString<32, 63>' requested here
  CopyString(pInfo->manufacturerID, TestManufacturerID);
  ^

which actually gives more information than before: it gives the length of
both buffers.

Comment 13

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8563225 [details] [diff] [review]
Properly initialize string fields from the PKCS#11 test module

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

LGTM - thanks!

Comment 14

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd34e4330662

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd34e4330662