Certificate Transparency: Remove the obsolete invalid sct status.

Certificate Transparency: Remove the obsolete invalid sct status.

Remove the obsolete enum value SCT_STATUS_INVALID which was replaced
by two distinct enum values.

To avoid crashing Chrome clients which have entries cached on disk
with the obsolete enum value, fail to de-serialize such cache entries.

This reverts commit 321ed2a53224c50af40387e8211726f8400ecad2.

BUG=640296, 634006, 640689
Committed: https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f
Cr-Commit-Position: refs/heads/master@{#418367}

[Eran Messeri, 2016-08-31 18:20:26]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 18:20:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eran Messeri, 2016-08-31 18:56:07]

eranm@chromium.org changed reviewers:
+ felt@chromium.org, nasko@chromium.org, rsleevi@chromium.org

[Eran Messeri, 2016-08-31 19:16:02]

nasko, felt, rsleevi: Please review this change that is related to estark's
change for fixing https://bugs.chromium.org/p/chromium/issues/detail?id=640296.

Note this reverts the original change for fixing the issue but modifies the
HttpResponseInfo de-serialization code to ensure the issue is not re-introduced.

[Eran Messeri, 2016-08-31 19:16:16]

eranm@chromium.org changed reviewers:
+ estark@chromium.org

[commit-bot: I haz the power, 2016-08-31 20:04:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 20:04:17]

Dry run: This issue passed the CQ dry run.

[estark, 2016-08-31 22:03:33]

lgtm

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
content/common/ssl_status_serialization.cc:42: case 2:
Is this necessary? Can we just fall through to line 46?

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
File net/http/http_response_info_unittest.cc (right):

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
net/http/http_response_info_unittest.cc:108:
EXPECT_FALSE(restored_response_info.InitFromPickle(pickle, &truncated));
I would find this test slightly more comforting if you had a check before or
after that InitFromPickle() returns true with a legitimate SCTVerifyStatus. (So
that it's obvious that InitFromPickle() fails due to the bad SCTVerifyStatus
rather than some other reason.)

[Ryan Sleevi, 2016-09-01 00:01:29]

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
content/common/ssl_status_serialization.cc:42: case 2:
On 2016/08/31 22:03:33, estark (OOO through Sept 12) wrote:
> Is this necessary? Can we just fall through to line 46?

+1

https://codereview.chromium.org/2294373002/diff/1/net/cert/sct_status_flags.h
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/2294373002/diff/1/net/cert/sct_status_flags.h...
net/cert/sct_status_flags.h:26: // different reasons an SCT could be invalid.
// Obsolete. Kept here to avoid reuse.
// SCT_STATUS_INVALID = 2,

(This matches how we've handled deprecation in net/base/net_error_list)

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
net/http/http_response_info.cc:247: return false;
Compare this with 286-289, as an example. This present approach can still lead
to crashes (as new values are introduced & such)

Something like CheckSCTStatus() seems better for this, but lifted propertly into
sct_status_flags.cc

NET_EXPORT bool IsValidSCTStatus(int status);

And then this check can defer to it, and CheckSCTStatus could be replaced with
it.

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
File net/http/http_response_info_unittest.cc (right):

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
net/http/http_response_info_unittest.cc:108:
EXPECT_FALSE(restored_response_info.InitFromPickle(pickle, &truncated));
On 2016/08/31 22:03:33, estark (OOO through Sept 12) wrote:
> I would find this test slightly more comforting if you had a check before or
> after that InitFromPickle() returns true with a legitimate SCTVerifyStatus.
(So
> that it's obvious that InitFromPickle() fails due to the bad SCTVerifyStatus
> rather than some other reason.)

+1

[Eran Messeri, 2016-09-01 14:14:40]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 14:14:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eran Messeri, 2016-09-01 14:16:35]

Ryan, PTAL - addressed all comments, as suggested, created a function for
checking if the SCTVerifyStatus value is valid.

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
File content/common/ssl_status_serialization.cc (right):

https://codereview.chromium.org/2294373002/diff/1/content/common/ssl_status_s...
content/common/ssl_status_serialization.cc:42: case 2:
On 2016/09/01 at 00:01:29, Ryan Sleevi (slow) wrote:
> On 2016/08/31 22:03:33, estark (OOO through Sept 12) wrote:
> > Is this necessary? Can we just fall through to line 46?
> 
> +1

Done.

https://codereview.chromium.org/2294373002/diff/1/net/cert/sct_status_flags.h
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/2294373002/diff/1/net/cert/sct_status_flags.h...
net/cert/sct_status_flags.h:26: // different reasons an SCT could be invalid.
On 2016/09/01 at 00:01:29, Ryan Sleevi (slow) wrote:
> // Obsolete. Kept here to avoid reuse.
> // SCT_STATUS_INVALID = 2,
> 
> (This matches how we've handled deprecation in net/base/net_error_list)

Done.

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info.cc
File net/http/http_response_info.cc (right):

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
net/http/http_response_info.cc:247: return false;
On 2016/09/01 at 00:01:29, Ryan Sleevi (slow) wrote:
> Compare this with 286-289, as an example. This present approach can still lead
to crashes (as new values are introduced & such)
> 
> Something like CheckSCTStatus() seems better for this, but lifted propertly
into sct_status_flags.cc
> 
> NET_EXPORT bool IsValidSCTStatus(int status);
> 
> And then this check can defer to it, and CheckSCTStatus could be replaced with
it.

Done.

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
File net/http/http_response_info_unittest.cc (right):

https://codereview.chromium.org/2294373002/diff/1/net/http/http_response_info...
net/http/http_response_info_unittest.cc:108:
EXPECT_FALSE(restored_response_info.InitFromPickle(pickle, &truncated));
On 2016/09/01 at 00:01:29, Ryan Sleevi (slow) wrote:
> On 2016/08/31 22:03:33, estark (OOO through Sept 12) wrote:
> > I would find this test slightly more comforting if you had a check before or
> > after that InitFromPickle() returns true with a legitimate SCTVerifyStatus.
(So
> > that it's obvious that InitFromPickle() fails due to the bad SCTVerifyStatus
> > rather than some other reason.)
> 
> +1

Good catch, thanks - turns out SCTs aren't serialized properly if there's no
cert.

[commit-bot: I haz the power, 2016-09-01 14:24:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 14:24:02]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nasko, 2016-09-01 17:00:29]

I don't understand this area as well as I wish, but happy to rubberstamp for
content/ once everyone else is happy with the CL.

[Ryan Sleevi, 2016-09-01 21:29:46]

LGTM % compile fix & style nit

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:8: #include "net/base/net_export.h"
#include <stdint>

#include "net/base/net_export.h"

(But see below)

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:45: NET_EXPORT bool IsValidSCTStatus(uint32_t
status);
STYLE: 
https://google.github.io/styleguide/cppguide.html#Integer_Types
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Option 1: IsValidSCTStatus(int status)
Option 2: C++11 enum-base for SCTVerifyStatus (line 17), as

enum SCTVerifyStatus : uint32_t {


The reason I highlight the mismatch is that SCTVerifyStatus is not a fixed
underlying type (e.g. int-or-implementation-defined-larger). Either approach
harmonizes them, but I've got a preference for Option 2 because that's
guaranteed-explicit :)

[Eran Messeri, 2016-09-09 09:18:28]

eranm@chromium.org changed reviewers:
- nasko@chromium.org

[Eran Messeri, 2016-09-09 09:26:04]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 09:26:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eran Messeri, 2016-09-09 09:38:14]

felt: Please review chrome/browser/ui/website_settings/website_settings.cc
rsleevi: Addressed your comment.
nasko to cc because the content/common changes are superfluous now that the
ssl_status_serialization code is gone.

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
File net/cert/sct_status_flags.h (right):

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:8: #include "net/base/net_export.h"
On 2016/09/01 at 21:29:46, Ryan Sleevi (slow) wrote:
> #include <stdint>
> 
> #include "net/base/net_export.h"
> 
> (But see below)

Done.

https://codereview.chromium.org/2294373002/diff/20001/net/cert/sct_status_fla...
net/cert/sct_status_flags.h:45: NET_EXPORT bool IsValidSCTStatus(uint32_t
status);
On 2016/09/01 at 21:29:46, Ryan Sleevi (slow) wrote:
> STYLE: 
> https://google.github.io/styleguide/cppguide.html#Integer_Types
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> 
> Option 1: IsValidSCTStatus(int status)
> Option 2: C++11 enum-base for SCTVerifyStatus (line 17), as
> 
> enum SCTVerifyStatus : uint32_t {
> 
> 
> The reason I highlight the mismatch is that SCTVerifyStatus is not a fixed
underlying type (e.g. int-or-implementation-defined-larger). Either approach
harmonizes them, but I've got a preference for Option 2 because that's
guaranteed-explicit :)

Done, used the 2nd option.

[commit-bot: I haz the power, 2016-09-09 09:54:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 09:54:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2016-09-09 18:35:22]

LGTM % compile fix

https://codereview.chromium.org/2294373002/diff/60001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/2294373002/diff/60001/net/net.gypi#newcode446
net/net.gypi:446: 'cert/sct_status_flags.cc',
You will need to move these two files up to line 133 to fix the nacl compilation
errors

[Eran Messeri, 2016-09-13 18:30:21]

The CQ bit was checked by eranm@chromium.org to run a CQ dry run

[Eran Messeri, 2016-09-13 18:30:32]

https://codereview.chromium.org/2294373002/diff/60001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/2294373002/diff/60001/net/net.gypi#newcode446
net/net.gypi:446: 'cert/sct_status_flags.cc',
On 2016/09/09 at 18:35:22, Ryan Sleevi (slow) wrote:
> You will need to move these two files up to line 133 to fix the nacl
compilation errors

Done.

[commit-bot: I haz the power, 2016-09-13 18:31:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 19:49:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 19:49:19]

Dry run: This issue passed the CQ dry run.

[felt, 2016-09-13 20:54:03]

lgtm

[Eran Messeri, 2016-09-13 20:56:00]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2016-09-13 20:56:01]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/2294373002/#ps80001
(title: "Making common headers available to nacl code")

[commit-bot: I haz the power, 2016-09-13 20:56:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 21:03:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-13 21:05:36]

Description was changed from

==========
Certificate Transparency: Remove the obsolete invalid sct status.

Remove the obsolete enum value SCT_STATUS_INVALID which was replaced
by two distinct enum values.

To avoid crashing Chrome clients which have entries cached on disk
with the obsolete enum value, fail to de-serialize such cache entries.

This reverts commit 321ed2a53224c50af40387e8211726f8400ecad2.

BUG=640296,634006,640689
==========

to

==========
Certificate Transparency: Remove the obsolete invalid sct status.

Remove the obsolete enum value SCT_STATUS_INVALID which was replaced
by two distinct enum values.

To avoid crashing Chrome clients which have entries cached on disk
with the obsolete enum value, fail to de-serialize such cache entries.

This reverts commit 321ed2a53224c50af40387e8211726f8400ecad2.

BUG=640296,634006,640689

Committed: https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f
Cr-Commit-Position: refs/heads/master@{#418367}
==========

[commit-bot: I haz the power, 2016-09-13 21:05:38]

Patchset 5 (id:??) landed as
https://crrev.com/b082a9ffa25c1bcb6e884379f0f3cf4e0997931f
Cr-Commit-Position: refs/heads/master@{#418367}

[Jeffrey Yasskin, 2016-09-15 19:10:13]

jyasskin@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Jeffrey Yasskin, 2016-09-15 19:10:14]

https://codereview.chromium.org/2294373002/diff/80001/net/http/http_response_...
File net/http/http_response_info_unittest.cc (right):

https://codereview.chromium.org/2294373002/diff/80001/net/http/http_response_...
net/http/http_response_info_unittest.cc:104: ct::GetX509CertSCT(&sct);
FYI, this fails to initialize the 'origin' field, which is then written to
response_info_, causing an MSan failure at
https://build.chromium.org/p/chromium.memory.full/builders/Linux%20MSan%20Tes...:

Uninitialized bytes in __msan_check_mem_is_initialized at offset 0 inside
[0x7ffcdf07e8b8, 4)
==31610==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xa7ce15a in WriteBytesCommon base/pickle.cc:478:3
    #1 0xa7ce15a in WriteBytesStatic<4> base/pickle.cc:443:0
    #2 0x98c5407 in WritePOD<int> base/pickle.h:367:5
    #3 0x98c5407 in WriteInt base/pickle.h:229:0
    #4 0x98c5407 in Persist net/cert/signed_certificate_timestamp.cc:43:0
    #5 0x991c383 in Persist net/http/http_response_info.cc:381:18
    #6 0x2cb75d0 in TestBody net/http/http_response_info_unittest.cc:111:18
    #7 0xa9af466 in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #8 0xa9af466 in Run testing/gtest/src/gtest.cc:2474:0
    #9 0xa9b2757 in Run testing/gtest/src/gtest.cc:2656:11
    #10 0xa9b3f7b in Run testing/gtest/src/gtest.cc:2774:28
    #11 0xa9d1121 in RunAllTests testing/gtest/src/gtest.cc:4647:43
    #12 0xa9d0124 in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2458:12
    #13 0xa9d0124 in Run testing/gtest/src/gtest.cc:4255:0
    #14 0xba78ae0 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #15 0xba78ae0 in Run base/test/test_suite.cc:246:0
    #16 0xba7d487 in Run base/callback.h:64:12
    #17 0xba7d487 in LaunchUnitTestsInternal
base/test/launcher/unit_test_launcher.cc:206:0
    #18 0xba7ccfe in LaunchUnitTests
base/test/launcher/unit_test_launcher.cc:445:10
    #19 0x5f4b8a7 in main net/test/run_all_unittests.cc:59:10
    #20 0x7f6a633157ec in __libc_start_main
/build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226:0
    #21 0x870af0 in _start ??:0

  Uninitialized value was stored to memory at
    #0 0x98c57fd in Persist net/cert/signed_certificate_timestamp.cc:43:3
    #1 0x991c383 in Persist net/http/http_response_info.cc:381:18
    #2 0x2cb75d0 in TestBody net/http/http_response_info_unittest.cc:111:18
    #3 0xa9af466 in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #4 0xa9af466 in Run testing/gtest/src/gtest.cc:2474:0
    #5 0xa9b2757 in Run testing/gtest/src/gtest.cc:2656:11
    #6 0xa9b3f7b in Run testing/gtest/src/gtest.cc:2774:28
    #7 0xa9d1121 in RunAllTests testing/gtest/src/gtest.cc:4647:43
    #8 0xa9d0124 in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2458:12
    #9 0xa9d0124 in Run testing/gtest/src/gtest.cc:4255:0
    #10 0xba78ae0 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #11 0xba78ae0 in Run base/test/test_suite.cc:246:0
    #12 0xba7d487 in Run base/callback.h:64:12
    #13 0xba7d487 in LaunchUnitTestsInternal
base/test/launcher/unit_test_launcher.cc:206:0
    #14 0xba7ccfe in LaunchUnitTests
base/test/launcher/unit_test_launcher.cc:445:10
    #15 0x5f4b8a7 in main net/test/run_all_unittests.cc:59:10
    #16 0x7f6a633157ec in __libc_start_main
/build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226:0

  Uninitialized value was created by a heap allocation
    #0 0x8da8c2 in operator new(unsigned long) ??:0
    #1 0xa68c2f8 in GetX509CertSCT net/test/ct_test_util.cc:220:14
    #2 0x2cb7451 in TestBody net/http/http_response_info_unittest.cc:104:3
    #3 0xa9af466 in HandleExceptionsInMethodIfSupported<testing::Test, void>
testing/gtest/src/gtest.cc:2458:12
    #4 0xa9af466 in Run testing/gtest/src/gtest.cc:2474:0
    #5 0xa9b2757 in Run testing/gtest/src/gtest.cc:2656:11
    #6 0xa9b3f7b in Run testing/gtest/src/gtest.cc:2774:28
    #7 0xa9d1121 in RunAllTests testing/gtest/src/gtest.cc:4647:43
    #8 0xa9d0124 in
HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>
testing/gtest/src/gtest.cc:2458:12
    #9 0xa9d0124 in Run testing/gtest/src/gtest.cc:4255:0
    #10 0xba78ae0 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:46
    #11 0xba78ae0 in Run base/test/test_suite.cc:246:0
    #12 0xba7d487 in Run base/callback.h:64:12
    #13 0xba7d487 in LaunchUnitTestsInternal
base/test/launcher/unit_test_launcher.cc:206:0
    #14 0xba7ccfe in LaunchUnitTests
base/test/launcher/unit_test_launcher.cc:445:10
    #15 0x5f4b8a7 in main net/test/run_all_unittests.cc:59:10
    #16 0x7f6a633157ec in __libc_start_main
/build/eglibc-oqps9y/eglibc-2.15/csu/libc-start.c:226:0

SUMMARY: MemorySanitizer: use-of-uninitialized-value
(/b/swarming/w/irP1cChJ/out/Release/net_unittests+0xa7ce15a)