928227 - [Wifi][WPA-EAP] prevent read built-in CAs through keystore socket.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

keystore socket doesn't put any limitation on getting CAs stored in NSS now.
So if you know the nickname of certain CA, you can get it out of NSS through keystore socket protocol.

Hence keystore is for wifi usage only, and it's CA must be imported first.
We can put some guard preventing users getting CAs they doesn't own.

For loose limitation, we can refuse request for getting CAs of nickname with prefix "Builtin Object Token:".
For strict limitation, we can only provide CA with prefix added by import function in bug 917102, maybe "WIFI_"

Comment 1

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - Only allow read certifiacte in NSS with prefix WIFI_SERVERCERT_.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

We should also limit the client belong wifi.wifi group to allow access to keyStore socket.

http://dxr.mozilla.org/mozilla-central/source/ipc/keystore/KeyStore.cpp?from=keystore.cpp#1

Comment 3

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: WIP - Add access control for keystore.

1. Provide socket fd to receive handler for permission control.
2. Only accept getting certificate with certain nickname prefix.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8413601 [details] [diff] [review]
WIP - Add access control for keystore.

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

I think you had the wrong Kyle there. However, since tzimmermann has been doing more work on unixsocket than I have lately, I'm deferring fb? to him.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Also, should this be a security bug instead of a MoCo-confidential bug?

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8413601 [details] [diff] [review]
WIP - Add access control for keystore.

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

Hi,

please see my comments below.

::: ipc/keystore/KeyStore.cpp
@@ +340,5 @@
> +    SendResponse(PERMISSION_DENIED);
> +    ResetHandlerInfo();
> +    return;
> +  }
> +

The top of this function does not belong into the receive code. You will re-run it on every incoming piece of data.

Instead, you should validate credentials once after the socket got connected. You should extend KeyStore::OnConnectSuccess to receive the file descriptor as argument.

::: ipc/unixsocket/UnixSocket.h
@@ +24,5 @@
>    // Number of octets in mData.
>    size_t mSize;
>    size_t mCurrentWriteOffset;
>    nsAutoArrayPtr<uint8_t> mData;
> +  int mFd;

The class is only for data.

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Argh, bugzilla removed most of the first comment's context! :( I was talking about moving validation out of |KeyStore::ReceiveSocketData|.

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0001. Add access control for keystore.

Address comment 7.

Comment 9

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: 0002. IPC changes due to change of unixsocket.

Add fd info as argument of UnixSocketConsumer::OnConnectSuccess(), ipcs using unixsocket are affected.

Comment 10

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Also, should this be a security bug instead of a MoCo-confidential bug?

I can't set bug into that group, so using MoCo-confidential instead...

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8422181 [details] [diff] [review]
0002. IPC changes due to change of unixsocket.

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

Sorry, I think you can implement access control in KeyStoreConnector::SetUp() without touching any other UnixSocket users.  Besides, as the name suggests, "a connector decides whether an just accepted socket should be valid" sounds reasonable for me.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422180 [details] [diff] [review]
0001. Add access control for keystore.

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

Hi Chuck,

In general f+ for the current code, but I like Vicamo's suggestion of moving the credential validation to the SetUp method. Please implement it this way and the end result should be even cleaner. Thanks.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

An can we please stop marking bugs as 'confidential,' just because they might contain a security-related bug?

Comment 14

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Add access control for keystore. V2

Address comment 11.

Thanks, vicamo!

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8422312 [details] [diff] [review]
Add access control for keystore. V2

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

Hi Chuck,

I'm not in charge of KeyStore.{cpp,h}, so I'm forwarding this to Kyle. Please have a look at my comments for things seem fix-worthy to me; nits mostly.

::: ipc/keystore/KeyStore.cpp
@@ +32,5 @@
>  namespace ipc {
>  
>  static const char* KEYSTORE_SOCKET_NAME = "keystore";
>  static const char* KEYSTORE_SOCKET_PATH = "/dev/socket/keystore";
> +static const char* allowedUsers[] = {

Since this is global static const, you might format this in all-capital letters, like KEYSTORE_* above.

@@ +37,5 @@
> +  "root",
> +  "wifi",
> +  NULL
> +};
> +static const char* allowedCertificatePrefixs[] = {

Same here.

@@ +90,5 @@
> +  // Socket permission check.
> +  struct ucred userCred;
> +  socklen_t len = sizeof(struct ucred);
> +
> +  if (aFd < 0 ||

Not necessary. getsockopt will handle that case (if that can even happen).

@@ +363,5 @@
> +            break;
> +          }
> +          int match = 0;
> +          for (const char **prefix = allowedCertificatePrefixs; *prefix; prefix++ ) {
> +            if (!strncmp(*prefix, certName, strlen(*prefix))) {

Use |strcmp| here. Currently, you're walking over *prefix twice.

@@ +368,5 @@
> +              match = 1;
> +              break;
> +            }
> +          }
> +          if (!match) {

You might remove |match| completely and test for !(*prefix) here.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8422312 [details] [diff] [review]
Add access control for keystore. V2

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

tzimmermann managed to cover most of the nits I'd have, so r=me.

Comment 17

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Add access control for keystore. V3. r=kmachulis

Address comment 15.

Thanks to all reviewers.

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=0c3a9f2d08f0

Comment 19

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Build on all platform and run test on emulator : https://tbpl.mozilla.org/?tree=Try&rev=d5dc65066b35

Comment 20

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Try looks good!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7f3c9587b559

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7f3c9587b559