Closed Bug 928227 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: Wifi, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox32 fixed, firefox-esr31 unaffected)

RESOLVED FIXED
2.0 S2 (23may)
tracking-b2g backlog
Tracking Status
firefox32 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: chucklee, Assigned: chucklee)

References

Details

(Whiteboard: [p=5][adv-main32-])

Attachments

(1 file, 5 obsolete files)

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_"
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
Group: mozilla-corporation-confidential
blocking-b2g: --- → backlog
1. Provide socket fd to receive handler for permission control.
2. Only accept getting certificate with certain nickname prefix.
Attachment #8340230 - Attachment is obsolete: true
Attachment #8413601 - Flags: feedback?(vchang)
Attachment #8413601 - Flags: feedback?(khuey)
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.
Attachment #8413601 - Flags: feedback?(khuey) → feedback?(tzimmermann)
Also, should this be a security bug instead of a MoCo-confidential bug?
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.
Attachment #8413601 - Flags: feedback?(tzimmermann) → feedback-
Argh, bugzilla removed most of the first comment's context! :( I was talking about moving validation out of |KeyStore::ReceiveSocketData|.
Address comment 7.
Attachment #8413601 - Attachment is obsolete: true
Attachment #8413601 - Flags: feedback?(vchang)
Attachment #8422180 - Flags: review?(tzimmermann)
Add fd info as argument of UnixSocketConsumer::OnConnectSuccess(), ipcs using unixsocket are affected.
Attachment #8422181 - Flags: review?(vyang)
Attachment #8422181 - Flags: review?(echou)
Attachment #8422181 - Flags: review?(allstars.chh)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #8422181 - Flags: review?(vyang) → review-
Group: mozilla-employee-confidential → core-security
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.
Attachment #8422180 - Flags: review?(tzimmermann) → review+
An can we please stop marking bugs as 'confidential,' just because they might contain a security-related bug?
Address comment 11.

Thanks, vicamo!
Attachment #8422180 - Attachment is obsolete: true
Attachment #8422181 - Attachment is obsolete: true
Attachment #8422181 - Flags: review?(echou)
Attachment #8422181 - Flags: review?(allstars.chh)
Attachment #8422312 - Flags: review?(tzimmermann)
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.
Attachment #8422312 - Flags: review?(tzimmermann)
Attachment #8422312 - Flags: review?(kyle)
Attachment #8422312 - Flags: feedback+
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.
Attachment #8422312 - Flags: review?(kyle) → review+
Whiteboard: [p=5]
Target Milestone: --- → 2.0 S2 (23may)
Whiteboard: [p=5] → [p=5][adv-main32-]
blocking-b2g: backlog → ---
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.