Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
694123002:
Limit lifetime of self-signed certificate used for TLS on Cast channel (Closed)
DescriptionLimit lifetime of self-signed certificate used for TLS on Cast channel
BUG=428920
R=mfoltz@chromium.org
Committed: https://crrev.com/9d04deed4364293a7f662d69cbbc23c4cab355f6
Cr-Commit-Position: refs/heads/master@{#308288}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Simplify the change. No need to make a new X509Certificate object. #
Total comments: 5
Patch Set 3 : Address Kevin Marshall comment. #Patch Set 4 : Handle some mfoltz comments #
Messages
Total messages: 20 (4 generated)
mfoltz@chromium.org changed reviewers: + marshallk@google.com
mfoltz@chromium.org changed required reviewers: + marshallk@google.com
Please add a unit test in the appropriate place, e.g. cast_auth_util_unittest.cc Please check with Kevin as he has a patch in progress adding openssl support to our authentication library. https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... extensions/browser/api/cast_channel/cast_socket.cc:166: // we need to check that the peer cert (which is self-signed) doesn't have an Please write a function in either cast_auth_util.cc (if the same code is applicable to NSS and openssl) or NSS and openssl implementations written in cast_auth_util_{nss,openssl}.cc https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... extensions/browser/api/cast_channel/cast_socket.cc:179: base::Time::Now() + base::TimeDelta::FromDays(2); This is not really a security feature as the system clock can be trivially set to bypass this check.
https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... extensions/browser/api/cast_channel/cast_socket.cc:166: // we need to check that the peer cert (which is self-signed) doesn't have an On 2014/11/01 20:17:34, mark a. foltz wrote: > Please write a function in either cast_auth_util.cc (if the same code is > applicable to NSS and openssl) or NSS and openssl implementations written in > cast_auth_util_{nss,openssl}.cc I set out to do this as you suggested, but it seems to require a disproportionate amount of new code to verify a simple time comparison check. I got so far as to add a function to cast_auth_util, but the body of the function is literally two lines long (I realized that the code had been more complicated than was necessary, and is simpler in the second draft). In order to add a unit test for this simple function, I would need to create and import one or two specially constructed X.509 certificates so that I can extract their expiry times and do a comparison with a known time. Note also that there are already unit tests for extracting the expiry time of a certificate in net/cert/x509_certificate_unittest.cc. I will do this if you still think it would be worthwhile. https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... extensions/browser/api/cast_channel/cast_socket.cc:179: base::Time::Now() + base::TimeDelta::FromDays(2); On 2014/11/01 20:17:34, mark a. foltz wrote: > This is not really a security feature as the system clock can be trivially set > to bypass this check. Yes, we know this is not a strong security check, and can be overridden by the user. However it is a simple change which should, in conjunction with some receiver changes, help mitigate a particular exploit, so we are making this change on all senders. There will likely be other changes.
On 2014/11/03 21:06:11, dougsteed wrote: > https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... > extensions/browser/api/cast_channel/cast_socket.cc:166: // we need to check that > the peer cert (which is self-signed) doesn't have an > On 2014/11/01 20:17:34, mark a. foltz wrote: > > Please write a function in either cast_auth_util.cc (if the same code is > > applicable to NSS and openssl) or NSS and openssl implementations written in > > cast_auth_util_{nss,openssl}.cc > > I set out to do this as you suggested, but it seems to require a > disproportionate amount of new code to verify a simple time comparison check. I > got so far as to add a function to cast_auth_util, but the body of the function > is literally two lines long (I realized that the code had been more complicated > than was necessary, and is simpler in the second draft). In order to add a unit > test for this simple function, I would need to create and import one or two > specially constructed X.509 certificates so that I can extract their expiry > times and do a comparison with a known time. Note also that there are already > unit tests for extracting the expiry time of a certificate in > net/cert/x509_certificate_unittest.cc. I will do this if you still think it > would be worthwhile. > > https://codereview.chromium.org/694123002/diff/1/extensions/browser/api/cast_... > extensions/browser/api/cast_channel/cast_socket.cc:179: base::Time::Now() + > base::TimeDelta::FromDays(2); > On 2014/11/01 20:17:34, mark a. foltz wrote: > > This is not really a security feature as the system clock can be trivially set > > to bypass this check. > > Yes, we know this is not a strong security check, and can be overridden by the > user. However it is a simple change which should, in conjunction with some > receiver changes, help mitigate a particular exploit, so we are making this > change on all senders. There will likely be other changes. Mark, can you take a look at my response to your comments?
Sorry for the delay. I would like Kevin to take a pass as well as he is the most familiar with the socket code at this time. I would like to see a unit test as well. Can a valid cert be hard coded with an expiry known to be in the past? https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:166: // we need to ensure that the peer cert (which is self-signed) doesn't have an Nit: Start sentence with "Ensure" https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:170: expiry > base::Time::Now() + base::TimeDelta::FromDays(2)) { Please declare a constant for "2" for the maximum lifetime in days. https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << "Peer cert has excessive lifetime. expiry=" << expiry; We'll want to log a socket event for an expired cert. - Add an enum value SSL_CERT_EXPIRED to the proto here: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... - Call logger_->LogSocketEventWithDetails(channel_id_, SSL_CERT_EXPIRED, ...) with the expiry time as the detail message https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:179: << *cert; Can you please remove the << *cert line while you are here? This dumps binary data into the text log which is not very helpful.
lgtm https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... File extensions/browser/api/cast_channel/cast_socket.cc (right): https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << "Peer cert has excessive lifetime. expiry=" << expiry; Also log the IP endpoint.
On 2014/11/21 00:41:13, Kevin Marshall wrote: > lgtm > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << "Peer cert > has excessive lifetime. expiry=" << expiry; > Also log the IP endpoint. Added the extra logging. Mark, are you OK with this now?
On 2014/12/01 22:49:27, dougsteed wrote: > On 2014/11/21 00:41:13, Kevin Marshall wrote: > > lgtm > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << "Peer > cert > > has excessive lifetime. expiry=" << expiry; > > Also log the IP endpoint. > > Added the extra logging. > Mark, are you OK with this now? There are several comments from my previous message that are not yet addressed.
On 2014/12/01 23:27:38, mark a. foltz wrote: > On 2014/12/01 22:49:27, dougsteed wrote: > > On 2014/11/21 00:41:13, Kevin Marshall wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << "Peer > > cert > > > has excessive lifetime. expiry=" << expiry; > > > Also log the IP endpoint. > > > > Added the extra logging. > > Mark, are you OK with this now? > > There are several comments from my previous message that are not yet addressed. Sorry Mark, Don't know how I missed those, looking at them now. You reiterated your comment about unit tests. Did you see my earlier response in #6 to your comment #4 on this subject ? This new code is about the self-signed cert, not about the client auth cert. So it is not analogous to the existing tests in cast_auth_util_unittests.cc. I could write a new test, but all it would do is verify that chromium net can read the expiry time out of a cert, and there are already such tests in net.
dougsteed@chromium.org changed reviewers: + rockot@chromium.org
On 2014/12/02 00:22:51, dougsteed wrote: > On 2014/12/01 23:27:38, mark a. foltz wrote: > > On 2014/12/01 22:49:27, dougsteed wrote: > > > On 2014/11/21 00:41:13, Kevin Marshall wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > > extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << > "Peer > > > cert > > > > has excessive lifetime. expiry=" << expiry; > > > > Also log the IP endpoint. > > > > > > Added the extra logging. > > > Mark, are you OK with this now? > > > > There are several comments from my previous message that are not yet > addressed. > > Sorry Mark, Don't know how I missed those, looking at them now. > You reiterated your comment about unit tests. Did you see my earlier response in > #6 to your comment #4 on this subject ? > This new code is about the self-signed cert, not about the client auth cert. So > it is not analogous to the existing tests in cast_auth_util_unittests.cc. > I could write a new test, but all it would do is verify that chromium net can > read the expiry time out of a cert, and there are already such tests in net. Mark, handled your comments, except for the unit test issue. Ken, At Mark's request I augmented the logging proto, which is in extensions/common/api/cast_channel. Need review from someone in extensions/OWNERS.
On 2014/12/02 21:38:20, dougsteed wrote: > On 2014/12/02 00:22:51, dougsteed wrote: > > On 2014/12/01 23:27:38, mark a. foltz wrote: > > > On 2014/12/01 22:49:27, dougsteed wrote: > > > > On 2014/11/21 00:41:13, Kevin Marshall wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > > > File extensions/browser/api/cast_channel/cast_socket.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/694123002/diff/20001/extensions/browser/api/c... > > > > > extensions/browser/api/cast_channel/cast_socket.cc:171: LOG(ERROR) << > > "Peer > > > > cert > > > > > has excessive lifetime. expiry=" << expiry; > > > > > Also log the IP endpoint. > > > > > > > > Added the extra logging. > > > > Mark, are you OK with this now? > > > > > > There are several comments from my previous message that are not yet > > addressed. > > > > Sorry Mark, Don't know how I missed those, looking at them now. > > You reiterated your comment about unit tests. Did you see my earlier response > in > > #6 to your comment #4 on this subject ? > > This new code is about the self-signed cert, not about the client auth cert. > So > > it is not analogous to the existing tests in cast_auth_util_unittests.cc. > > I could write a new test, but all it would do is verify that chromium net can > > read the expiry time out of a cert, and there are already such tests in net. > > Mark, handled your comments, except for the unit test issue. > Ken, At Mark's request I augmented the logging proto, which is in > extensions/common/api/cast_channel. Need review from someone in > extensions/OWNERS. lgtm
LGTM
The CQ bit was checked by dougsteed@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/694123002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9d04deed4364293a7f662d69cbbc23c4cab355f6 Cr-Commit-Position: refs/heads/master@{#308288} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
