Closed Bug 1168298 Opened 5 years ago Closed 4 years ago

[Bluetooth] Support OBEX authentication procedure

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2r+, firefox45 fixed, b2g-master affected)

RESOLVED FIXED
FxOS-S11 (13Nov)
feature-b2g 2.2r+
Tracking Status
firefox45 --- fixed
b2g-master --- affected

People

(Reporter: jaliu, Assigned: ben.tian)

References

Details

Attachments

(3 files, 10 obsolete files)

9.64 KB, patch
shawnjohnjr
: review+
mrbkap
: review+
Details | Diff | Splinter Review
45.26 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
1.06 KB, patch
Details | Diff | Splinter Review
According to PBAP spec., the OBEX authentication procedure is required by PSE.

Gecko have to handle related OBEX headers, including 'Authentication Challenge' and 'Authentication Response'.
Blocks: 892179
See Also: → 1168266
Attached patch bug1168298_ObexAuth_v0.patch (obsolete) — Splinter Review
Assignee: nobody → jaliu
Mark feature-b2g: 2.2r+ since PBAP is required bluetooth feature.
feature-b2g: --- → 2.2r+
Assignee: 6jamin → wiwang
Stead this bug from Will.
Assignee: wiwang → btian
Attachment #8649743 - Attachment is obsolete: true
Rebased on the latest m-c with some revision. Need tests on m-c and 2.2r.
Depends on: 1203821
Attachment #8668869 - Attachment is obsolete: true
Attachment #8672957 - Flags: review?(shuang)
Attachment #8668870 - Attachment is obsolete: true
Attachment #8672958 - Flags: review?(shuang)
Attachment #8672958 - Attachment description: Patch 2: OBEX authentication challenge - dom/bluetooth change → Patch 2 (v1): OBEX authentication challenge - dom/bluetooth change
Comment on attachment 8672957 [details] [diff] [review]
Patch 1 (v1): Support OBEX authentication procedure - webidl change

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

LGTM. But a few comments addressed before I r+ this.

::: dom/base/nsGkAtomList.h
@@ +771,5 @@
>  GK_ATOM(ondrain, "ondrain")
>  GK_ATOM(ondrop, "ondrop")
>  GK_ATOM(oneitbroadcasted, "oneitbroadcasted")
>  GK_ATOM(onenabled, "onenabled")
> +GK_ATOM(onenterobexpincodereq, "onenterobexpincodereq")

It's a bit strange for me to call onenterobexpincodereq.
Following IrObex 3.5.1, this is actually related to 'password' instead of pin code that GAP defined, right?
I think this makes people confused pin code and Authenticate Challenge password because it's OBEX protocol not Bluetooth.
How about onobexpasswordreq? Since your later patch contain user id, that means you're trying to align with obex spec. Any reason you called it pin code?

::: dom/webidl/BluetoothAdapter.webidl
@@ +29,5 @@
>    // playing time (ms)
>    long long   position = -1;
>    // one of 'STOPPED'/'PLAYING'/'PAUSED'/'FWD_SEEK'/'REV_SEEK'/'ERROR'
>    DOMString   playStatus = "";
>  };

Too many unrelated changes (and this is even webidl change). I don't like too much unrelated change if it's not related to your main purpose. Please remove BluetoothAdapter.webidl from this patch and open new bug for format/style change.

::: dom/webidl/BluetoothObexAuthHandle.webidl
@@ +10,5 @@
> +   * Reply pin code for enterobexpincodereq. The promise will be rejected if the
> +   * operation fails.
> +   */
> +  [NewObject]
> +  Promise<void> setPinCode(DOMString aPinCode);

Promise<void> setPassword(DOMString aPassword);
Attachment #8672957 - Flags: review?(shuang)
Revise per reviewer's comment 8.
Attachment #8672957 - Attachment is obsolete: true
Attachment #8673443 - Flags: review?(shuang)
Revise per patch 1 (v2) change.
Attachment #8672958 - Attachment is obsolete: true
Attachment #8672958 - Flags: review?(shuang)
Attachment #8673444 - Flags: review?(shuang)
Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

Blake, can you help this bluetooth webidl change?

The change adds an event handler to inform gaia app of password request for authentication. Gaia app can then call BluetoothObexAuthHandle.setPassword/reject to set password for or reject authentication. According gecko implementation is in patch 2 of this bug.
Attachment #8673443 - Flags: review?(mrbkap)
Comment on attachment 8673444 [details] [diff] [review]
Patch 2 (v2): OBEX authentication challenge - dom/bluetooth change

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

Some comments addressed, I have some questions that need your response. Thanks!

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +741,5 @@
>    index += AppendHeaderWho(&res[index], kObexLeastMaxSize,
>                             kPbapObexTarget.mUuid, sizeof(BluetoothUuid));
>    index += AppendHeaderConnectionId(&res[index], 0x01);
>  
> +  // authentication response

We shall add some comments just like we did for other AppParameters.
Such as "[headerId:1][length:2][appParam:var]" in order to give the clear picture of response packet layout.

@@ +768,5 @@
> +                                 mObexRemoteNonce, DIGEST_LENGTH);
> +
> +    index += AppendAuthResponse(&res[index], kObexLeastMaxSize - index,
> +                                digestResponse, offset);
> +  }

What happened when |aPassword.IsEmpty()|?
Looking into IrOBEX 1.2, 3.5 Authentication Procedure.

"The server authenticates the client using the same basic algorithm. When a client attempts an operation
to the server which requires authentication, the server sends an Authenticate Challenge header along
with an UNAUTHORIZED response code in a response packet."

@@ +774,1 @@
>    SendObexData(res, ObexResponseCode::Success, index);

We shall handle reject case with UNAUTHORIZED response code in a response packet for failure case.


Response with opcode "UNAUTHORIZED" and Authenticate Challenge, as 3.5.4 Authentication Examples shows.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +16,2 @@
>  #include "ObexBase.h"
> + 

nit: remove the trailing space.

@@ +82,5 @@
> +  /**
> +   * Reply vCard objects to IPC / in-process 'pullphonebook' request.
> +   *
> +   * @param aActor         [in]  (IPC) blob actor of the vCard objects
> +   * @param aBlob          [in]  (in-process) blob of the vCard objects

The comment doesn't seems to be consistent with declaration. Perhaps typo?

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +1651,5 @@
> +      NS_LITERAL_STRING("Failed to reject OBEX authentication request"));
> +    return;
> +  }
> +
> +  pbap->ReplyToAuthChallenge(EmptyString());

A bit strange here for me. If we want to reject ObexAuth, do we still send obex response code success, after checking ReplyToAuthChallenge?

::: dom/bluetooth/common/ObexBase.h
@@ +283,5 @@
> +      }
> +    }
> +  }
> +
> +  void GetAuthResponse(uint8_t** aRetData, int* aRetDataLength) const

I checked Patch1 and Patch2, I did not see |GetAuthResponse| been used. Is it for future usage? I would like to see some comments for purpose of this function.

::: dom/bluetooth/common/webapi/BluetoothAdapter.cpp
@@ +1357,5 @@
> +  init.mHandle = BluetoothObexAuthHandle::Create(GetOwner());
> +
> +  // The optional userId of authenticate challenge
> +  if (arr.Length() == 1 && arr[0].value().type() == BluetoothValue::TnsString) {
> +    init.mUserId = arr[0].value().get_nsString();

I don't see where we actually pass userid in other places (in BluetoothPbapManager). Do we expect to add userid related code in BluetoothPbapManager?
Attachment #8673444 - Flags: review?(shuang)
Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

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

Hi Ben,
Sorry for late notice.
But I think you missed test_interfaces.html and that may break some mochitest test cases.
Please add events into test_interfaces.html.
Comment on attachment 8673443 [details] [diff] [review]
Patch 1 (v2): Support OBEX authentication procedure - webidl change

Cancel review first for comment 13.
Attachment #8673443 - Flags: review?(mrbkap)
Add tests per comment 13.
Attachment #8673443 - Attachment is obsolete: true
Attachment #8673568 - Flags: review?(shuang)
Comment on attachment 8673568 [details] [diff] [review]
Patch 1 (v3): Support OBEX authentication procedure - webidl change

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

Thanks, Ben!
Attachment #8673568 - Flags: review?(shuang) → review+
Comment on attachment 8673568 [details] [diff] [review]
Patch 1 (v3): Support OBEX authentication procedure - webidl change

Blake, can you help this bluetooth webidl change?

The change adds an event handler to inform gaia app of password request for authentication. Gaia app can then call BluetoothObexAuthHandle.setPassword/reject methods to set password for or reject authentication. Corresponding gecko implementation is in patch 2 of this bug.
Attachment #8673568 - Flags: review?(mrbkap)
Attachment #8673568 - Flags: review?(mrbkap) → review+
Attachment #8673444 - Attachment is obsolete: true
Attachment #8681183 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> Looking into IrOBEX 1.2, 3.5 Authentication Procedure.
> 
> "The server authenticates the client using the same basic algorithm. When a
> client attempts an operation
> to the server which requires authentication, the server sends an
> Authenticate Challenge header along
> with an UNAUTHORIZED response code in a response packet."

The paragraph above is for "server to authenticate client", in contrast to "client to authenticate server" that we want to support. I revised patch in comment 18 to respond "unauthorized" to reject authentication as IrOBEX doesn't specify how server rejects authentication from client.

Let me know for any further thoughts.
(In reply to Ben Tian [:btian] from comment #18)
> Created attachment 8681183 [details] [diff] [review]
> Patch 2 (v3): OBEX authentication challenge - dom/bluetooth change

Note I've verified this patch on MecApp tool to connect with authentication challenge from client to server.
Comment on attachment 8681183 [details] [diff] [review]
Patch 2 (v3): OBEX authentication challenge - dom/bluetooth change

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +750,5 @@
> +  if (!aPassword.IsEmpty()) {
> +    // Section 3.5.2.1 "Request-digest", PBAP 1.2
> +    // The request-digest is required and calculated as follows:
> +    //   H(nonce ":" password)
> +    char hashString[kObexLeastMaxSize];

Is it true that we should use kObexLeastMaxSize? Just wonder IrObex defined the size of hash string.

@@ +757,5 @@
> +    hashString[offset++] = ':';
> +
> +    memcpy(&hashString[offset], NS_ConvertUTF16toUTF8(aPassword).get(),
> +           aPassword.Length());
> +    offset += aPassword.Length();

Should check |aPassword.Length()| less than |kObexLeastMaxSize|.
Attachment #8681183 - Flags: review?(shuang)
Changes:
- allocate |hashString| with dynamic length
- precisify length of |digestResonse|
- rebase on bug 1221477
Attachment #8681183 - Attachment is obsolete: true
Attachment #8683535 - Flags: review?(shuang)
Depends on: 1221477
Comment on attachment 8683535 [details] [diff] [review]
Patch 2 (v4): OBEX authentication challenge - dom/bluetooth change

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +520,5 @@
> +
> +  // Get authentication challenge data
> +  uint8_t* dataPtr;
> +  int dataLength;
> +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);

GetAuthChallenge, this function leaks memory. 
nsAutoArrayPtr<uint8_t> dataPtr;

@@ +521,5 @@
> +  // Get authentication challenge data
> +  uint8_t* dataPtr;
> +  int dataLength;
> +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);
> +

Please add comments to point out the format of packets for fetching nonce.

::: dom/bluetooth/common/ObexBase.h
@@ +275,5 @@
> +
> +    for (int i = 0; i < length; ++i) {
> +      if (mHeaders[i]->mId == ObexHeaderId::AuthChallenge) {
> +        uint8_t* ptr = mHeaders[i]->mData.get();
> +        *aRetData = new uint8_t[mHeaders[i]->mDataLength];

When will aRetData be released?
Attachment #8683535 - Flags: review?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> ::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
> @@ +520,5 @@
> > +
> > +  // Get authentication challenge data
> > +  uint8_t* dataPtr;
> > +  int dataLength;
> > +  aHeader.GetAuthChallenge(&dataPtr, &dataLength);
> 
> GetAuthChallenge, this function leaks memory. 
> nsAutoArrayPtr<uint8_t> dataPtr;

nsAutoArrayPtr cannot be passed directly into GetAuthChallenge. Need more time to revise the code.

../../../../gecko/central/dom/bluetooth/bluedroid/BluetoothPbapManager.cpp:524:49: error: no matching function for call to 'mozilla::dom::bluetooth::ObexHeaderSet::GetAuthChallenge(nsAutoArrayPtr<unsigned char>*, int*) const'
Revise per comment 23.

Change:
- use nsAutoArrayPtr to get authentication challenge data
- add comment to explain nonce format
Attachment #8683535 - Attachment is obsolete: true
Attachment #8684743 - Flags: review?(shuang)
Comment on attachment 8684743 [details] [diff] [review]
Patch 2 (v5): OBEX authentication challenge - dom/bluetooth change

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

::: dom/bluetooth/common/ObexBase.cpp
@@ +77,5 @@
>                        aWho, aLength);
>  }
>  
>  int
> +AppendAuthChallenge(uint8_t* aRetBuf, int aBufferSize, const uint8_t* aDigest,

Do you have any reason to keep this function? It seems to be unused now.

@@ +85,5 @@
> +                      aDigest, aLength);
> +}
> +
> +int
> +AppendAuthResponse(uint8_t* aRetBuf, int aBufferSize, const uint8_t* aDigest,

It seems to be unused now.
Attachment #8684743 - Flags: review?(shuang)
Attachment #8684743 - Attachment is obsolete: true
Attachment #8684821 - Flags: review?(shuang)
https://hg.mozilla.org/mozilla-central/rev/9a942f34debf
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S11 (13Nov)
You need to log in before you can comment on or make changes to this bug.