1296138 - Support Bluetooth Low Energy FIDO U2F tokens on Windows

Status: NEW

(Core :: DOM: Device Interfaces, enhancement, P3)

Description

[J.C. Jones [:jcj] (he/him)]

There is native Bluetooth Low Energy support on Windows, and an outside contributor has offered to implement BLE FIDO tokens via the Windows API as a compatible nsIU2FToken in dom/u2f/

Comment 1

[Johan Verrept]

Attachment: U2F BLEToken, with Windows hardware support

Hi,

VASCO Datasecurity is contributing this patch to add BLE U2F Token support to Firefox.

The patch contains:
- an interface patch for the U2FToken interface to add appId to IsRegistered() function.
- changes to U2FRegisterTask and U2FSignTask to move some repeated hashes outside the loop.
- adds a preference security.webauth.u2f_enable_bletoken to enable the BLE Token
- Generic U2F BLE Token support
- U2FBLEToken Windows backend.


Some things that still need attention in the patch:
- The code only works on Windows 8.0 or higher. This should take care of itself as implemented but perhaps someone better acquainted with windows and Firefox build system should verify this is done according to the rules.
- there is certainly room for a more generic way to generate U2F raw messages that can be shared between transports. current implementation could be prettier.

Limitations:
- because of the current architecture of the U2F subsystem, this code slow things down for some time if multiple U2F BLE tokens are registered (each token has its own timeout).
- paired tokens are detected when instantiating the U2F object. Changes are not detected.

From my discussions with jcj, these will solve themselves as the U2F subsystem is re-implemented.

PS, I ran clang-format on all files, my apologies for the white space changes.

Comment 2

[Johan Verrept]

Attachment: contentparent.patch

ContentParent.cpp update for new nsIU2FToken interface.

Comment 3

[Johan Verrept]

Attachment: bletokenwindows_memleak.patch

Fixed a memory leak when reading the ControlPointLength characteristic.

Comment 4

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8784831 [details] [diff] [review]
U2F BLEToken, with Windows hardware support

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

Thanks so much for this, Johan and VASCO. I'm still reviewing the BLETokenWindows.* files, but please take a look at the comments below to start. Don't worry overmuch about the formatting; we don't use clang-format, but it'll be easy to pick up your changes to the existing code.

The new files, we'll just reformat them as a last-pass before committing. I can handle that, too.

I should be able to review the remaining BLETokenWindows.* files over this upcoming weekend, despite the US holiday.

::: dom/u2f/BLEToken.cpp
@@ +71,5 @@
> +    MutexAutoLock lock(mMutex);
> +
> +    // should never happen
> +    if (mReplyBuffer != nullptr)
> +      return NS_ERROR_UNEXPECTED;

Global: Our style requires brackets around even 1-line conditionals; true for a bunch of lines in this file.

@@ +89,5 @@
> +  if (retval != NS_OK)
> +    return retval;
> +
> +  // get a buffer
> +  segment = static_cast<uint8_t*>(malloc(controlPointLength));

Is there some sanity checking we can do here, to avoid getting a full 2^32 allocation?

@@ +100,5 @@
> +  // start segmenting
> +  sequence = 0;
> +
> +  // initialize first packet
> +  segment[0] = aCmd;

I'd generally prefer this to use CryptoBuffer or an nsTArray<uint8> than operate on raw memory. Take a look at http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSU2FToken.cpp#617 for an example.

@@ +153,5 @@
> +    // wait for reply
> +    while (!mCompleted) {
> +      retval = mCommandInProgress.Wait();
> +      if (retval != NS_OK)
> +        return retval;

Don't we need to clean up the mReplyBuffer and its length here, too?

@@ +158,5 @@
> +    }
> +
> +    // error during reassembly
> +    if (mReplyReturnValue != NS_OK)
> +      return mReplyReturnValue;

And here?

@@ +177,5 @@
> +BLEToken::IncomingData(const uint8_t* buffer, uint32_t bufferLen)
> +{
> +  unsigned int l = 0;
> +  MutexAutoLock lock(mMutex);
> +

Probably a good idea to assert that mReplyBuffer is not null before getting into this.

@@ +197,5 @@
> +    if (buffer[0] == FIDO_BLE_CMD_KEEPALIVE)
> +      return;
> +
> +    // extract header information
> +    mExpected = ((short)(buffer[1] << 8)) | (buffer[2] & 0xFF);

Don't use the C-style cast

@@ +207,5 @@
> +      return;
> +    }
> +
> +    // store command
> +    mReplyCmd = (BLEFidoCommand)buffer[0];

Don't use this C-style cast

@@ +249,5 @@
> +    mCommandInProgress.Notify();
> +    return;
> +  }
> +  // copy data
> +  memcpy(mReplyBuffer + mReceived, buffer + bufferLen - l, l);

Definitely need to be sure mReplyBuffer is not-null here, but asserting at the beginning seems fine since we're under the Mutex.

@@ +333,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(result);
> +  nsresult retval;
> +
> +  // real work.

?

@@ +355,5 @@
> +  *result = false;
> +
> +  // length ok?
> +  if (replyLen != version.Length())
> +    goto leave;

Please don't use Goto for cleanups this small. We only permit goto when it's really outrageous.

@@ +359,5 @@
> +    goto leave;
> +
> +  // string matches?
> +  for (uint32_t i = 0; i < replyLen; i++)
> +    if (reply[i] != version[i])

Why not strncmp?

@@ +395,5 @@
> +      return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  // real work.
> +  uint8_t buffer[7 + 64 + 1 + 255 + 2];

Can you document where these sizes are from? Something akin to http://searchfox.org/mozilla-central/source/security/manager/ssl/nsNSSU2FToken.cpp#522 ?

@@ +401,5 @@
> +  uint8_t challenge[32];
> +  uint32_t challengeLen = 32;
> +  U2FResultCode resultCode;
> +
> +  // FIXME real random?

Grab a Random Generator: http://searchfox.org/mozilla-central/source/dom/base/Crypto.cpp#96

@@ +518,5 @@
> +
> +  return BLESendCommand(buffer, bufferLen, resultCode, signature, signatureLen);
> +}
> +
> +// must be called under lock

I've been cheating and using ReentrantMonitor, which has wait/notify semantics as well without needing a condition variable (unless you need it to be separate).

@@ +563,5 @@
> +  // calculate result code
> +  uint32_t returnedResult =
> +    (((uint32_t)buffer[bufferLength - 2]) << 8) | (buffer[bufferLength - 1]);
> +  switch (returnedResult) {
> +    case 0x9000:

Can we document where these come from, too?

@@ +587,5 @@
> +  // return data result if requested and present
> +  if (resultBuffer && resultBufferLength) {
> +    if (bufferLength > 2) {
> +      *resultBufferLength = bufferLength - 2;
> +      *resultBuffer = (uint8_t*)malloc(*resultBufferLength);

C++ style cast, please

@@ +602,5 @@
> +nsresult
> +BLEToken::Initialize(Sequence<Authenticator>& pAuthenticators)
> +{
> +#ifdef U2F_BLETOKEN_WINDOWS
> +  BLETokenWindows::Init();

Probably should make sure this doesn't NS_FAILED()

::: dom/u2f/BLEToken.h
@@ +46,5 @@
> +  BLEToken();
> +  ~BLEToken();
> +
> +protected:
> +  enum BLEFidoCommand

It would be good to toss a comment in here with a link to the spec where these are defined.

::: security/manager/ssl/nsIU2FToken.idl
@@ +25,1 @@
>     * @param keyHandle Key Handle to evaluate.

Need to update the documentation for this, too.

Comment 5

[Johan Verrept]

> Global: Our style requires brackets around even 1-line conditionals; true
> for a bunch of lines in this file.

Will fix.
 
> @@ +89,5 @@
> > +  if (retval != NS_OK)
> > +    return retval;
> > +
> > +  // get a buffer
> > +  segment = static_cast<uint8_t*>(malloc(controlPointLength));
> 
> Is there some sanity checking we can do here, to avoid getting a full 2^32
> allocation?

Yes. It must be between 20 and 512. Will fix.
 
> @@ +100,5 @@
> > +  // start segmenting
> > +  sequence = 0;
> > +
> > +  // initialize first packet
> > +  segment[0] = aCmd;
> 
> I'd generally prefer this to use CryptoBuffer or an nsTArray<uint8> than
> operate on raw memory. Take a look at
> http://searchfox.org/mozilla-central/source/security/manager/ssl/
> nsNSSU2FToken.cpp#617 for an example.

Will use the nsTArray<uint8_t>

> @@ +153,5 @@
> > +    // wait for reply
> > +    while (!mCompleted) {
> > +      retval = mCommandInProgress.Wait();
> > +      if (retval != NS_OK)
> > +        return retval;
> 
> Don't we need to clean up the mReplyBuffer and its length here, too?
> 
> @@ +158,5 @@
> > +    }
> > +
> > +    // error during reassembly
> > +    if (mReplyReturnValue != NS_OK)
> > +      return mReplyReturnValue;
> 
> And here?

Ouch. Yes.
 
> @@ +177,5 @@
> > +BLEToken::IncomingData(const uint8_t* buffer, uint32_t bufferLen)
> > +{
> > +  unsigned int l = 0;
> > +  MutexAutoLock lock(mMutex);
> > +
> 
> Probably a good idea to assert that mReplyBuffer is not null before getting
> into this.

Currently the function just returns if it receives data when mReplyBuffer is nullptr. The data is ignored.

I'm a bit worried about adding an ASSERT. 

If we start sending a command and we get back an error, we stop sending. However, it is possible that there are still buffers at OS level who are not yet sent out to the device and which will trigger other error replies from the device. By this time, we have already aborted the CommandWrite() function and mReplyBuffer is nullptr, triggering the ASSERT.

Any programming mistakes we made would already be shown by the initial error return value. All the subsequent errors that trigger the ASSERT are meaningless.

I consciously made the choice to just ignore unexpected data instead of ASSERT for it but if you insist I will make the change.

> Don't use the C-style cast

Will replace them all.

> @@ +249,5 @@
> > +    mCommandInProgress.Notify();
> > +    return;
> > +  }
> > +  // copy data
> > +  memcpy(mReplyBuffer + mReceived, buffer + bufferLen - l, l);
> 
> Definitely need to be sure mReplyBuffer is not-null here, but asserting at
> the beginning seems fine since we're under the Mutex.

We are under lock and mReplyBuffer is checked to be non-null first thing after establishing the lock.
 
> @@ +333,5 @@
> > +{
> > +  NS_ENSURE_ARG_POINTER(result);
> > +  nsresult retval;
> > +
> > +  // real work.
> 

real work after sanity checking the parameters. Will disappear.

> @@ +355,5 @@
> > +  *result = false;
> > +
> > +  // length ok?
> > +  if (replyLen != version.Length())
> > +    goto leave;
> 
> Please don't use Goto for cleanups this small. We only permit goto when it's
> really outrageous.

Will fix.
 
> @@ +359,5 @@
> > +    goto leave;
> > +
> > +  // string matches?
> > +  for (uint32_t i = 0; i < replyLen; i++)
> > +    if (reply[i] != version[i])
> 
> Why not strncmp?

reply is uint8_t, version is uint16_t, this works and avoids having to figure out how to do that compare in a mozilla approved way...
 
> @@ +395,5 @@
> > +      return NS_ERROR_UNEXPECTED;
> > +  }
> > +
> > +  // real work.
> > +  uint8_t buffer[7 + 64 + 1 + 255 + 2];
> 
> Can you document where these sizes are from? Something akin to
> http://searchfox.org/mozilla-central/source/security/manager/ssl/
> nsNSSU2FToken.cpp#522 ?

I will remove all this code and create a few nice and clean classes to create generic FIDO raw messages using a nsTArray.
 
> @@ +401,5 @@
> > +  uint8_t challenge[32];
> > +  uint32_t challengeLen = 32;
> > +  U2FResultCode resultCode;
> > +
> > +  // FIXME real random?
> 
> Grab a Random Generator:
> http://searchfox.org/mozilla-central/source/dom/base/Crypto.cpp#96

I left this as a question... 

There isn't actually any need for this value to be true random, the value isn't actually used. It isn't set to 0 in case a device sanity checks it although 0 is supposedly a valid value. 

Getting a random generator and a real random value is a lot more expensive for, as far as I can tell, no good reason.

Perhaps we could just change the interface again and pass in the challenge we have available at the higher layer. Probably the best option.

Let me know what you prefer.

> @@ +518,5 @@
> > +
> > +  return BLESendCommand(buffer, bufferLen, resultCode, signature, signatureLen);
> > +}
> > +
> > +// must be called under lock
> 
> I've been cheating and using ReentrantMonitor, which has wait/notify
> semantics as well without needing a condition variable (unless you need it
> to be separate).

I will have to look at the ReentrantMonitor code to figure this out. I will leave this as an optional improvement for now.

> @@ +563,5 @@
> > +  // calculate result code
> > +  uint32_t returnedResult =
> > +    (((uint32_t)buffer[bufferLength - 2]) << 8) | (buffer[bufferLength - 1]);
> > +  switch (returnedResult) {
> > +    case 0x9000:
> 
> Can we document where these come from, too?

I've added a link at the top where you can download the BLE specs. Linking to a specific document might break. FIDO doesn't seem to consider the direct links to these document permanent.
 
> @@ +602,5 @@
> > +nsresult
> > +BLEToken::Initialize(Sequence<Authenticator>& pAuthenticators)
> > +{
> > +#ifdef U2F_BLETOKEN_WINDOWS
> > +  BLETokenWindows::Init();
> 
> Probably should make sure this doesn't NS_FAILED()

Not necessary. The Init function will return an error value but this can be perfectly normal, for example, when this code is run on Windows 7 and the loading of the Windows BLE DLLs fails.

> ::: security/manager/ssl/nsIU2FToken.idl
> @@ +25,1 @@
> >     * @param keyHandle Key Handle to evaluate.
> 
> Need to update the documentation for this, too.

Will fix.


Most of the changes are implemented but I will do some testing on Monday before updating the patch.

Comment 6

[Johan Verrept]

Attachment: BLEToken.r2.patch

U2F BLE Token with Windows hardware support, r2

I addresses most issues mentioned above, cleaned things up and added more MOZ_ASSERTS.

Changes to previous patch:
* removed all C style casts.
* added bracket around every 1 line conditional
* verify controlPointLength is between 20 and 512 (instead of up to 2^16)
* replaced all raw memory usage at BLEToken level to using wrapped nsTArray<uint8_t>
* removed explicit memory allocation as much as possible
* Added creation classes for U2F Raw Messagses. request and reply.
* Added wrapper classes for U2F Segmentation and Reassembly, suitable for BLE and USB.
* fixed documentation in nsIU2FToken.idl
* reformated to 2 spaces, no tabs.

Open issues:
* what to use as challenge parameter in IsRegistered() function.
* in depth validation of U2F reply messages.

Comment 7

[Johan Verrept]

Attachment: modify_timeout.patch

Additional small change that increases the timeout in the CommandWrite wait. 500ms is probably too short for some BLE devices.