Verify that V4 updates were applied correctly by computing a checksum on the final result

RESOLVED FIXED in Firefox 52

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: francois, Assigned: dimi)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: #sbv4-m2)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
After parsing and applying a list update, we need to compare the end result with the expected checksum:

"The checksum lets clients verify that the local database has not suffered any corruption. If the checksum does not match, the client must clear the database and reissue an update with an empty state field."

https://developers.google.com/safe-browsing/v4/update-api#http-post-response

It's not clear what exactly the checksum is computed on.
(Reporter)

Comment 1

3 years ago
We should also store that checksum on disk.

When we load the database from file, we should compute a checksum on the loaded data and compare it with that stored checksum.

Dimi says that the V2 HashStore already does this so we only need to add this to V4.
(Reporter)

Updated

3 years ago
Blocks: 1285848
Comment hidden (typo)

Comment 3

3 years ago
FWIW 
Checksum : "The expected SHA256 hash of the client state; that is, of the sorted list of all hashes present in the database after applying the provided update. If the client state doesn't match the expected state, the client must disregard this update and retry later".
(Reporter)

Updated

3 years ago
No longer blocks: safebrowsingv4
(Reporter)

Updated

3 years ago
Depends on: 1305484
(Reporter)

Updated

3 years ago
Blocks: 1305780
(Assignee)

Comment 4

3 years ago
Posted patch WIP Patch (obsolete) — Splinter Review
(Assignee)

Comment 5

3 years ago
Attachment #8797097 - Attachment is obsolete: true
Attachment #8798287 - Flags: feedback?(hchang)
(Assignee)

Comment 6

3 years ago
(In reply to Dimi Lee[:dimi][:dlee] from comment #5)
> Created attachment 8798287 [details] [diff] [review]
> Apply check sum

This part doesn't contain check "checksum" when loading prefixset from disk
Comment on attachment 8798287 [details] [diff] [review]
Apply check sum

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

Pretty neat and complete patch! The test case is fairly complete, too :) 
Just leave some reminders 

1) TableUpdateV4::Checksum related things need to be removed after Bug 1305484 lands.

2) If the server doesn't respond with checksum, what we can do might be calculating 
   on our own and saving to meta data to at least the on-disk tables are not corrupted.
   Not sure if this is useful and worth doing.

BTW, I am thinking if we can combine full update and partial update code 
since it seems to me that a full update is a partial update with empty
initial lists and no removal indices.
Attachment #8798287 - Flags: feedback?(hchang) → feedback+
(Assignee)

Updated

3 years ago
Blocks: 1308384
(Assignee)

Comment 8

3 years ago
Thanks for review !

> 2) If the server doesn't respond with checksum, what we can do might be
> calculating 
>    on our own and saving to meta data to at least the on-disk tables are not
> corrupted.
>    Not sure if this is useful and worth doing.
> 

I think it is useful because then we can always know if prefix file is corrupted even there is no checksum passed from google server.
File bug 1308384.

> BTW, I am thinking if we can combine full update and partial update code 
> since it seems to me that a full update is a partial update with empty
> initial lists and no removal indices.

Yes, i think they can be merged. Thanks for the suggestion.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1308384
(Assignee)

Updated

3 years ago
Attachment #8798726 - Flags: review?(francois)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8798727 [details]
Bug 1305581 - P2. Testcase for verifying checksum when update.

cancel review because i'll implement all checksum stuffs in this bug
- check checksum when update
- check checksum when load prefix set
- store self-calculate checksum in file when there is no checksum in update respnse
Attachment #8798727 - Flags: review?(francois)
(Assignee)

Updated

3 years ago
Whiteboard: #sbv4-m1 → #sbv4-m2
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Attachment #8798727 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Hi Francois,
This patch is based on Henry's patch in Bug 1305484, I set it to r? so you could check this after reviewing bug 1305484 if you have time, thanks!
(Reporter)

Comment 15

3 years ago
mozreview-review
Comment on attachment 8798726 [details]
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.

https://reviewboard.mozilla.org/r/84152/#review85658

This seems quite good to me. I've got a few inline comments.

One thing that would be nice to add to the tests is a hard-coded checksum in one of the test cases. Right now the test case uses the same code (`CryptoHash`) to compute the hash as the actual code. I think a hard-coded test case would allow us to catch regressions in `CryptoHash` or other similar mistakes.

::: toolkit/components/telemetry/Histograms.json:3830
(Diff revision 2)
>      "alert_emails": ["safebrowsing-telemetry@mozilla.org"],
>      "expires_in_version": "58",
>      "kind": "enumerated",
>      "n_values": 10,
>      "bug_numbers": [1305801],
> -    "description": "An error was encountered while parsing a partial update returned by a Safe Browsing V4 server (0 = addition of an already existing prefix, 1 = parser got into an infinite loop, 2 = removal index out of bounds)"
> +    "description": "An error was encountered while parsing a partial update returned by a Safe Browsing V4 server (0 = addition of an already existing prefix, 1 = parser got into an infinite loop, 2 = removal index out of bounds, 3 = checksum is not matched after applying update, 4 = update response doesn't contain checksum)"

I would suggest these shorter descriptions:

- 3 = checksum mismatch
- 4 = missing checksum

::: toolkit/components/url-classifier/Classifier.cpp:963
(Diff revision 2)
>      return NS_ERROR_FAILURE;
>    }
>  
>    nsresult rv = NS_OK;
>  
>    // prefixes2 is only used in partial update. If there are multiple

You swapped `prefixes1` and `prefixes2`, I think this comment needs to be updated too?

::: toolkit/components/url-classifier/Classifier.cpp:993
(Diff revision 2)
>        // If both prefix sets are empty, this means we are doing a partial update
>        // without a prior full/partial update in the loop. In this case we should
>        // get prefixes from the lookup cache first.
>        if (prefixes1.IsEmpty() && prefixes2.IsEmpty()) {
>          lookupCache->GetPrefixes(prefixes1);
>          input = &prefixes1;

This is now done above, I think these two lines are redundant.

::: toolkit/components/url-classifier/LookupCacheV4.h:58
(Diff revision 2)
> +
>    enum UPDATE_ERROR_TYPES {
>      DUPLICATE_PREFIX = 0,
>      INFINITE_LOOP = 1,
>      WRONG_REMOVAL_INDICES = 2,
> +    INCONSIST_CHECKSUM = 3,

`CHECKSUM_MISMATCH`

::: toolkit/components/url-classifier/LookupCacheV4.h:59
(Diff revision 2)
>    enum UPDATE_ERROR_TYPES {
>      DUPLICATE_PREFIX = 0,
>      INFINITE_LOOP = 1,
>      WRONG_REMOVAL_INDICES = 2,
> +    INCONSIST_CHECKSUM = 3,
> +    NO_CHECKSUM_IN_UPDATE_RESPONSE = 4,

`MISSING_CHECKSUM`

::: toolkit/components/url-classifier/LookupCacheV4.cpp:131
(Diff revision 2)
> +    return rv;
> +  }
> +
> +  rv = VerifyChecksum(checksum);
> +  if (NS_FAILED(rv)) {
> +    return rv;

This is the case we'll be in if there's file corruption. Can we reset the database or do something else that will cause a full update to happen immediately?

Also, it would be great to add some telemetry to see how often this happens. So maybe something like `URLCLASSIFIER_LC_LOAD_VALID_CHECKSUM` (true = correct checksum, false = checksum mismatch).

I'm not sure if "LC" is the right prefix to use here, so you should double-check that the other `URLCLASSIFIER_LC_*` telemetry applies to the same class/area of the code.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:213
(Diff revision 2)
> +
> +      // Compare the smallest string in old prefix set and add prefix set,
> +      // merge the smaller one into new map to ensure merged string still
> +      // follows lexigraphic order.
> +      pickOld = smallestOldPrefix < smallestAddPrefix;
> +    } else if (isOldMapEmpty ^ isAddMapEmpty) {

I find it hard to read this XOR and understand the implications of it.

Maybe we should check for `isOldMapEmpty && isAddMapEmpty` first to make this `if` more readily understandable?

::: toolkit/components/url-classifier/LookupCacheV4.cpp:268
(Diff revision 2)
> +  nsAutoCString checksum;
> +  crypto->Finish(false, checksum);
> +  if (aTableUpdate->Checksum().IsEmpty()) {
> +    Telemetry::Accumulate(Telemetry::URLCLASSIFIER_UPDATE_ERROR_TYPE,
> +                          NO_CHECKSUM_IN_UPDATE_RESPONSE);
> +

Maybe we should have a warning here too: "Update checksum missing."

::: toolkit/components/url-classifier/LookupCacheV4.cpp:275
(Diff revision 2)
> +    // checksum in .metadata
> +    std::string stdChecksum(checksum.BeginReading(), checksum.Length());
> +    aTableUpdate->NewChecksum(stdChecksum);
> +
> +  } else if (aTableUpdate->Checksum() != checksum){
> +    NS_WARNING("Checksum is not matched after applying partial update");

I would phrase this as "Checksum mismatch after applying partial update"

::: toolkit/components/url-classifier/LookupCacheV4.cpp:285
(Diff revision 2)
> +
> +  return NS_OK;
> +}
> +
> +already_AddRefed<nsICryptoHash>
> +LookupCacheV4::InitCrypto()

Maybe we should expose the error codes that come from CryptoHash (if any).

This method would then return an `nsresult` and would take the pointer as an output parameter.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:307
(Diff revision 2)
> +nsresult
> +LookupCacheV4::VerifyChecksum(const nsACString& aChecksum)
> +{
> +  nsCOMPtr<nsICryptoHash> crypto = InitCrypto();
> +  if (!crypto) {
> +    return NS_ERROR_FAILURE;

We could return the error from CryptoHash, as suggested above.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:329
(Diff revision 2)
> +
> +  nsAutoCString checksum;
> +  crypto->Finish(false, checksum);
> +
> +  if (checksum != aChecksum) {
> +    NS_WARNING("Checksum is not matched when load prefix from file");

"Checksum mismatch when loading prefixes from file."

::: toolkit/components/url-classifier/LookupCacheV4.cpp:330
(Diff revision 2)
> +  nsAutoCString checksum;
> +  crypto->Finish(false, checksum);
> +
> +  if (checksum != aChecksum) {
> +    NS_WARNING("Checksum is not matched when load prefix from file");
> +    return NS_ERROR_FAILURE;

If you can find an error that's related to mismatch checksums, that would be good. It would allow us to treat this case differently in `LoadFromFile()`.

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:1
(Diff revision 2)
>  #include "Classifier.h"

Looks like we forgot to add a copyright header here.

You can steal the one from http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/url-classifier/tests/gtest/TestRiceDeltaDecoder.cpp#1-2

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:159
(Diff revision 2)
> +    tableUpdate->NewRemovalIndices(removal->Elements(), removal->Length());
> +  }
> +
> +  if (checksum) {
> +    std::string stdChecksum;
> +    stdChecksum.assign((char*)checksum->BeginReading(), checksum->Length());

Should probably use a C++-style cast here.

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:275
(Diff revision 2)
> +    rv = cache.Open();
> +    ASSERT_EQ(rv, NS_OK);
> +  });
> +}
>  
> +// Testings start from here.

nit: "Tests" or "Testing"

I don't think "testings" is grammatically correct.

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:627
(Diff revision 2)
> +  }
> +
> +  Clear();
> +}
> +
> +TEST(UrlClassifierTableUpdateV4, InconsistChecksum)

"ChecksumMismatch"

::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:633
(Diff revision 2)
> +{
> +  _PrefixArray fArray, pArray;
> +  PrefixStringMap fMap, pMap;
> +  nsCString checksum;
> +
> +  {

If you're opening block scopes in order to isolate the tests, I think the variables (fMap, pMap, checksum) should be declared within the block scopes).
Attachment #8798726 - Flags: review?(francois) → review-
Comment hidden (mozreview-request)
(Reporter)

Comment 17

3 years ago
mozreview-review
Comment on attachment 8798726 [details]
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.

https://reviewboard.mozilla.org/r/84152/#review86366

I pointed out a few minor things we can simplify. The only important change here is the telemetry. I think we'll get better data if we clearly deal with unexpected errors.

::: toolkit/components/telemetry/Histograms.json:3830
(Diff revisions 2 - 3)
> +  "URLCLASSIFIER_VLPS_LOAD_VALID_CHECKSUM": {
> +    "alert_emails": ["safebrowsing-telemetry@mozilla.org"],
> +    "expires_in_version": "58",
> +    "kind": "boolean",
> +    "bug_numbers": [1305581],
> +    "description": "This metric is recorded every time variable length prefix set is loaded frmo disk, `false` is recorded if checksum mismatch"

Typo: "frmo"

Here's an alternate way to phrase this:

"Whether or not a variable-length prefix loaded from disk passed the checksum test (false = checksum mismatch)."

::: toolkit/components/url-classifier/LookupCacheV4.cpp:134
(Diff revisions 2 - 3)
>    rv = VerifyChecksum(checksum);
>    if (NS_FAILED(rv)) {
> +    if (rv == NS_ERROR_FILE_CORRUPTED) {
> +      Telemetry::Accumulate(Telemetry::URLCLASSIFIER_VLPS_LOAD_VALID_CHECKSUM, false);
> +    }
>      return rv;

Here we have a small problem. We're only keeping track of corrupt checksums and valid ones. We're missing this state:

- verification failed but for an unexpected reason

Maybe we should change the telemetry so that it's looking for corrupt checksums v. total number of checksum verifications? `URLCLASSIFIER_VLPS_LOAD_CORRUPT`

That way we would have two states:

- corrupt = true if `rv == NS_ERROR_FILE_CORRUPTION`
- corrupt = false if `rv` has any other value.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:138
(Diff revisions 2 - 3)
> +    }
>      return rv;
>    }
> +  Telemetry::Accumulate(Telemetry::URLCLASSIFIER_VLPS_LOAD_VALID_CHECKSUM, true);
>  
>    return NS_OK;

We can remove the `return rv` from the previous `if` statement and then change this line to be `return rv;`.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:210
(Diff revisions 2 - 3)
>      bool pickOld;
> +
> +    // If both prefix sets are not empty, then compare to find the smaller one.
>      if (!isOldMapEmpty && !isAddMapEmpty) {
>        if (smallestOldPrefix == smallestAddPrefix) {
> -        NS_WARNING("Add prefix should not exist in the original prefix set.");
> +        LOG(("Add prefix should not exist in the original prefix set."));

That's good. We should make sure we have some logging available in non-debug builds.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:220
(Diff revisions 2 - 3)
>  
>        // Compare the smallest string in old prefix set and add prefix set,
>        // merge the smaller one into new map to ensure merged string still
>        // follows lexigraphic order.
>        pickOld = smallestOldPrefix < smallestAddPrefix;
> -    } else if (isOldMapEmpty ^ isAddMapEmpty) {
> +    // If Old map is not empty and add map is empty, pick from old map.

Thanks for splitting this up. I think we can drop the comment here because it's just repeating the `if` clause.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:223
(Diff revisions 2 - 3)
>        // follows lexigraphic order.
>        pickOld = smallestOldPrefix < smallestAddPrefix;
> -    } else if (isOldMapEmpty ^ isAddMapEmpty) {
> -      pickOld = isAddMapEmpty;
> -    } else {
> +    // If Old map is not empty and add map is empty, pick from old map.
> +    } else if (!isOldMapEmpty && isAddMapEmpty) {
> +      pickOld = true;
> +    // If Old map is empty and add map is not empty, pick from add map.

Ditto here, we'd just be repeating the `if` clause.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:226
(Diff revisions 2 - 3)
> -      pickOld = isAddMapEmpty;
> -    } else {
> +    } else if (!isOldMapEmpty && isAddMapEmpty) {
> +      pickOld = true;
> +    // If Old map is empty and add map is not empty, pick from add map.
> +    } else if (isOldMapEmpty && !isAddMapEmpty) {
> +      pickOld = false;
> -      // If both maps are empty, then partial update is complete.
> +    // If both maps are empty, then partial update is complete.

That's a useful comment, thanks.

::: toolkit/components/url-classifier/LookupCacheV4.cpp:306
(Diff revisions 2 - 3)
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> -    return nullptr;
> +    return rv;
>    }
>  
> -  rv = crypto->Init(nsICryptoHash::SHA256);
> +  rv = aCrypto->Init(nsICryptoHash::SHA256);
>    if (NS_WARN_IF(NS_FAILED(rv))) {

I guess we can simplify this by dropping the `if` statement and only keeping the `NS_WARN_IF(NS_FAILED(rv))` bit.
Attachment #8798726 - Flags: review?(francois) → review-
(Reporter)

Comment 18

3 years ago
mozreview-review-reply
Comment on attachment 8798726 [details]
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.

https://reviewboard.mozilla.org/r/84152/#review86366

> Here we have a small problem. We're only keeping track of corrupt checksums and valid ones. We're missing this state:
> 
> - verification failed but for an unexpected reason
> 
> Maybe we should change the telemetry so that it's looking for corrupt checksums v. total number of checksum verifications? `URLCLASSIFIER_VLPS_LOAD_CORRUPT`
> 
> That way we would have two states:
> 
> - corrupt = true if `rv == NS_ERROR_FILE_CORRUPTION`
> - corrupt = false if `rv` has any other value.

I should note that this was my fault since you implemented exactly the telemetry I originally asked for :)
(Reporter)

Comment 19

3 years ago
mozreview-review-reply
Comment on attachment 8798726 [details]
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.

https://reviewboard.mozilla.org/r/84152/#review86366

> I should note that this was my fault since you implemented exactly the telemetry I originally asked for :)

Also, I got the name of the error code wrong. It should be `NS_ERROR_FILE_CORRUPTED` like you already have in your patch. No need to change this.
(Reporter)

Updated

3 years ago
Blocks: 1309770
Comment hidden (mozreview-request)
(Reporter)

Comment 21

3 years ago
mozreview-review
Comment on attachment 8798726 [details]
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.

https://reviewboard.mozilla.org/r/84152/#review87410
Attachment #8798726 - Flags: review?(francois) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 23

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1509b970b24f
Verify that V4 updates were applied correctly by computing a checksum on the final result. r=francois
Keywords: checkin-needed

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1509b970b24f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.