Closed Bug 1305581 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P2)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: francois, Assigned: dimi)

References

Details

(Whiteboard: #sbv4-m2)

Attachments

(2 files, 2 obsolete files)

Apply check sum
34.27 KB, patch
hchang
: feedback+
Details | Diff | Splinter Review
Bug 1305581 - Verify that V4 updates were applied correctly by computing a checksum on the final result.
58 bytes, text/x-review-board-request
francois
: review+
Details 
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.
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.
Blocks: 1285848
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".
No longer blocks: safebrowsingv4
Depends on: 1305484
Blocks: 1305780
Attached patch WIP Patch (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Apply check sum
        — 
          — Splinter Review
      

    


  

        Attachment #8797097 -
        Attachment is obsolete: true

        Attachment #8798287 -
        Flags: feedback?(hchang)

    
    

    
  
(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+

    
  
Blocks: 1308384

    
    

    
  
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.

    
    

    
      
  


  

    
    

    
      
  


  

    
  

        Attachment #8798726 -
        Flags: review?(francois)

    
    

    
  
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)

    
  
Whiteboard: #sbv4-m1 → #sbv4-m2

    
    

    
      
  

    
  

        Attachment #8798727 -
        Attachment is obsolete: true

    
    

    
  
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!

    
    

    
  
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 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-

    
    

    
  
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 :)

    
    

    
  
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.

    
  
Blocks: 1309770

    
    

    
      
  

    
    

    
  
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+

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1509b970b24f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago

          status-firefox52:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: