Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(33)

Issue 6338003: Explicitly logging out the token to avoid leaving it in a non-stable state. (Closed)

Created:
9 years, 11 months ago by Nelson Araujo
Modified:
9 years, 7 months ago
Reviewers:
rginda, kmixter1, nelsona
CC:
chromium-os-reviews_chromium.org, gauravsh, oritm
Visibility:
Public.

Description

Explicitly logging out the token to avoid leaving it in a non-stable state. Just calling session.close() will, sometimes, leave the TPM in an inconsistent state which will cause it to generate a bad RSA key, despite reporting key generation successful. Further use of the key will generate incorrect signatures. Problem does not reproduce all the time, and the likelihood of happening appears to vary depending on the hardware used (Cr-48 > AGZ > L13). Calling session.logout() before session.close() fixes the issue. Change-Id: Ib601818e180243ecf1f59e40f46d141e1d826286 BUG=chromium-os:10536 TEST=enroll a certificate and successfully connect to Google-A on Cr-48 device. Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=6a86083

Patch Set 1 #

Patch Set 2 : Adding helper method 'logoutAndClose'. #

Total comments: 4

Patch Set 3 : Returning early on logout failure. #

Patch Set 4 : Returning early on logout failure. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -11 lines) Patch
M base_policy/policy-utils.js View 1 8 chunks +12 lines, -10 lines 0 comments Download
M crypto_pkcs11.h View 1 2 chunks +7 lines, -1 line 0 comments Download
M crypto_pkcs11.cc View 1 2 4 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Nelson Araujo
9 years, 11 months ago (2011-01-13 23:56:18 UTC) #1
rginda
On 2011/01/13 23:56:18, Nelson Araujo wrote: If the correct usage is always "session.logout(); session.close();", then ...
9 years, 11 months ago (2011-01-14 17:54:43 UTC) #2
Nelson Araujo
On 2011/01/14 17:54:43, rginda wrote: > On 2011/01/13 23:56:18, Nelson Araujo wrote: > > If ...
9 years, 11 months ago (2011-01-14 22:36:07 UTC) #3
kmixter1
FYI, generally looks good, but will defer on this one to rginda. http://codereview.chromium.org/6338003/diff/4001/base_policy/policy-utils.js File base_policy/policy-utils.js ...
9 years, 11 months ago (2011-01-18 18:48:34 UTC) #4
Nelson Araujo
It would be great to close/push this CL today as Orit indicated a dev channel ...
9 years, 11 months ago (2011-01-18 21:56:58 UTC) #5
rginda
http://codereview.chromium.org/6338003/diff/4001/crypto_pkcs11.cc File crypto_pkcs11.cc (right): http://codereview.chromium.org/6338003/diff/4001/crypto_pkcs11.cc#newcode549 crypto_pkcs11.cc:549: OkOrThrow(C_Logout(session_handle_)); You should return early if this throws.
9 years, 11 months ago (2011-01-18 22:10:19 UTC) #6
Nelson Araujo
PTAL. http://codereview.chromium.org/6338003/diff/4001/crypto_pkcs11.cc File crypto_pkcs11.cc (right): http://codereview.chromium.org/6338003/diff/4001/crypto_pkcs11.cc#newcode549 crypto_pkcs11.cc:549: OkOrThrow(C_Logout(session_handle_)); On 2011/01/18 22:10:19, rginda wrote: > You ...
9 years, 11 months ago (2011-01-18 22:50:19 UTC) #7
rginda
LGTM On 2011/01/18 22:50:19, Nelson Araujo wrote: > PTAL. > > http://codereview.chromium.org/6338003/diff/4001/crypto_pkcs11.cc > File crypto_pkcs11.cc ...
9 years, 11 months ago (2011-01-18 22:54:31 UTC) #8
kmixter1
If the implementation does not logout, couldn't this cause the same bug? (Aka, is this ...
9 years, 11 months ago (2011-01-19 16:26:57 UTC) #9
Nelson Araujo
On 2011/01/19 16:26:57, kmixter1 wrote: > If the implementation does not logout, couldn't this cause ...
9 years, 11 months ago (2011-01-19 16:46:05 UTC) #10
nelsona
9 years, 11 months ago (2011-01-19 16:47:04 UTC) #11
On Wed, Jan 19, 2011 at 8:26 AM, Ken Mixter <kmixter@chromium.org> wrote:

> If the implementation does not logout, couldn't this cause the same bug?
>  (Aka, is this worthy of filing a bug and fixing the implementation?)
>
>
Yes, but only if you forget to call logout(). Agree it might be worth filing
the bug.


>
> On Tue, Jan 18, 2011 at 1:56 PM, <nelsona@chromium.org> wrote:
>
>> It would be great to close/push this CL today as Orit indicated a dev
>> channel
>> build would be created today and having this fix would be great.
>>
>> Please let me know if any issues remain ASAP.
>>
>>
>>
>>
>> http://codereview.chromium.org/6338003/diff/4001/base_policy/policy-utils.js
>> File base_policy/policy-utils.js (right):
>>
>>
>>
http://codereview.chromium.org/6338003/diff/4001/base_policy/policy-utils.js#...
>> base_policy/policy-utils.js:819: token.closeAllSessions();
>> On 2011/01/18 18:48:34, kmixter1 wrote:
>>
>>> Does this also log out of active sessions?
>>>
>>
>> Yes and No. Yes (per spec/theory). No (per implementation/practice).
>>
>>
>> http://codereview.chromium.org/6338003/
>>
>
>

Powered by Google App Engine
This is Rietveld 408576698