1276595 - Parse SafeBrowsing v4 update response

Status: RESOLVED DUPLICATE of bug 1274112

(Toolkit :: Safe Browsing, defect)

Comment 1

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1276595-Parse-update-response.patch

Comment 2

[Henry Chang [:hchang]]

Attachment: WIP Patch

Comment 3

[Henry Chang [:hchang]]

Attachment: Test case

Comment 4

[Henry Chang [:hchang]]

Attachment: Part 1: Parse update response

Comment 5

[Henry Chang [:hchang]]

Attachment: Part 2: Test case

Comment 6

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1276595-Use-protobuf-API-to-parse-v4-update-resp.patch

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8771369 [details] [diff] [review]
0001-Bug-1276595-Use-protobuf-API-to-parse-v4-update-resp.patch

Hi Dimi,

Could you give some feedback regarding this patch for parsing the update response? The parsing part is nothing but using protobuf API to obtain update info, which may include more than one update instructions.

Besides, I add ProtocolParser::End() and call it in DBService::FinishStream to let the protocolParser know when to start parsing the protobuf content.

Thanks :)

Comment 8

[Dimi Lee [:dimi]]

Comment on attachment 8771369 [details] [diff] [review]
0001-Bug-1276595-Use-protobuf-API-to-parse-v4-update-resp.patch

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

Looks good to me, thanks!
Just want to see some questions answered before f+

::: toolkit/components/url-classifier/ProtocolParser.cpp
@@ +116,5 @@
>  
>  nsresult
> +ProtocolParser::End()
> +{
> +  if (!mTableUpdate) {

As discussed we may not be able to use mTableUpdate here because it might be null.
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#277

@@ +129,5 @@
> +    return NS_OK;
> +  }
> +
> +  return ProcessProtobufData();
> +}

One question, will we have ProtocolParserV4 ?
If yes, do you plan to have different End function for v2 and v4 and put functions below to ProtocolParserV4 ?

@@ +156,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCString listName;
> +  bool isFullUpdate = false;

declare when use?

@@ +204,5 @@
> +
> +  for (int i = 0; i < aUpdate.size(); i++) {
> +    auto update = aUpdate.Get(i);
> +    if (!update.has_compression_type()) {
> +      NS_WARNING("Removal with no compression type.");

Not quite understand the warning message here, why only removal ?

@@ +243,5 @@
> +    return NS_OK;
> +  }
> +
> +  auto prefixes = rawHashes.raw_hashes();
> +  if (4 == rawHashes.prefix_size()) {

I think we can use the define here
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/Entries.h#26

@@ +249,5 @@
> +    uint32_t* fixedLengthPrefixes = (uint32_t*)prefixes.c_str();
> +    size_t numOfFixedLengthPrefixes = prefixes.size() / 4;
> +    PARSER_LOG(("* Raw addition (4)"));
> +    PARSER_LOG(("  - # of prefixes: %d", numOfFixedLengthPrefixes));
> +    PARSER_LOG(("  - Memory address: 0x%p", fixedLengthPrefixes));

Maybe add another check for full hash

Comment 9

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1276595-Use-protobuf-API-to-parse-v4-update-resp.patch

Comment 10

[Dimi Lee [:dimi]]

Comment on attachment 8772313 [details] [diff] [review]
0001-Bug-1276595-Use-protobuf-API-to-parse-v4-update-resp.patch

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

Looks great! like the design of ProtocolParserProtobuf.

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug 1276595 - Use protobuf API to parse v4 update response.

Review commit: https://reviewboard.mozilla.org/r/65182/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65182/

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug1276595.patch

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65182/diff/1-2/

Comment 14

[François Marier [:francois]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

https://reviewboard.mozilla.org/r/65182/#review64686

::: toolkit/components/url-classifier/ProtocolParser.h:17
(Diff revision 2)
>  
>  namespace mozilla {
>  namespace safebrowsing {
>  
>  /**
>   * Some helpers for parsing the safe

Let's update this (incomplete) comment to point out it's for V2:

    * Helpers to parse the "shavar", "digest256" and "simple" list formats

::: toolkit/components/url-classifier/ProtocolParser.h:39
(Diff revision 2)
>  
>    nsresult Begin();
> -  nsresult AppendStream(const nsACString& aData);
> +  virtual nsresult AppendStream(const nsACString& aData);
> +
> +  // Notify that the inbound data is ready for parsing if progressive
> +  // parsing is not supported.

nit: "parsing is not supported." -> "parsing is not supported, for example in V4."

::: toolkit/components/url-classifier/ProtocolParser.h:124
(Diff revision 2)
>    nsTArray<TableUpdate*> mTableUpdates;
>    // Updates to apply to the current table being parsed.
>    TableUpdate *mTableUpdate;
>  };
>  
> +// For parsing protobuf data.

nit: "Helpers to parse the "proto" list format"

::: toolkit/components/url-classifier/ProtocolParser.cpp:120
(Diff revision 2)
>  }
>  
>  nsresult
> +ProtocolParser::End()
> +{
> +  // Inbound data has been processed in every AppendStream() call.

nit: "data has been processed" -> "data has already been processed"

::: toolkit/components/url-classifier/ProtocolParser.cpp:747
(Diff revision 2)
> +ProtocolParserProtobuf::ProcessOneResponse(const ListUpdateResponse& aResponse)
> +{
> +  // A response must have a threat type.
> +  if (!aResponse.has_threat_type()) {
> +    NS_WARNING("Threat type not initialized. This seems to be an invalid response.");
> +    return NS_ERROR_FAILURE;

Maybe we should ignore (with a warning) unknown treat types instead of aborting the whole process?

I would expect `return NS_OK;` so that we keep looking at the other list updates but ignore this one.

::: toolkit/components/url-classifier/ProtocolParser.cpp:759
(Diff revision 2)
> +  nsresult rv = urlUtil->ConvertThreatTypeToListName(aResponse.threat_type(),
> +                                                     listName);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING(nsPrintfCString("Threat type to list name conversion error: %d",
> +                               aResponse.threat_type().get()));
> +    return NS_ERROR_FAILURE;

Again, maybe we should ignore (with a warning) unknown treat types instead of aborting the whole process?

I would expect `return NS_OK;` so that we keep looking at the other list updates but ignore this one.

::: toolkit/components/url-classifier/ProtocolParser.cpp:768
(Diff revision 2)
> +  bool isFullUpdate = false;
> +  if (aResponse.has_response_type()) {
> +    isFullUpdate =
> +      aResponse.response_type() == ListUpdateResponse::FULL_UPDATE;
> +  } else {
> +    NS_WARNING("Response type not initialized.");

Is this warning necessary or can we assume that if the response type isn't present then it's not a full update?

::: toolkit/components/url-classifier/ProtocolParser.cpp:773
(Diff revision 2)
> +    NS_WARNING("Response type not initialized.");
> +  }
> +
> +  // Warn if there's no new state.
> +  if (!aResponse.has_new_client_state()) {
> +    NS_WARNING("New state not initialized.");

Shouldn't this be a fatal condition? We can't apply the updates to the database if we don't know what client state value to set at the end.

Again, I would expect `return NS_OK;` so that we keep looking at the other list updates but ignore this one.

::: toolkit/components/url-classifier/ProtocolParser.cpp:779
(Diff revision 2)
> +  }
> +
> +  PARSER_LOG(("==== Update for threat type '%d' ====", aResponse.threat_type()));
> +  PARSER_LOG(("* listName: %s\n", listName.get()));
> +  PARSER_LOG(("* newState: %s\n", aResponse.new_client_state().c_str()));
> +  PARSER_LOG(("* isFullUpdate: %d\n", isFullUpdate));

You could also do:

    "* isFullUpdate: %s\n", isFullUpdate ? "yes" : "no"

::: toolkit/components/url-classifier/ProtocolParser.cpp:812
(Diff revision 2)
> +      ret = (aIsAddition ? ProcessRawAddition(update)
> +                         : ProcessRawRemoval(update));
> +      break;
> +
> +    case RICE:
> +      // Bug 1285848 - [SafeBrowsing] Supports encoded table update for v4.

nit: I would avoid having the bug title in there because it could change. Maybe just:

    // Not implemented yet (see bug 1285848)

::: toolkit/components/url-classifier/ProtocolParser.cpp:840
(Diff revision 2)
> +  auto prefixes = rawHashes.raw_hashes();
> +  if (4 == rawHashes.prefix_size()) {
> +    // Process fixed length prefixes separately.
> +    uint32_t* fixedLengthPrefixes = (uint32_t*)prefixes.c_str();
> +    size_t numOfFixedLengthPrefixes = prefixes.size() / 4;
> +    PARSER_LOG(("* Raw addition (4)"));

nit: I woud suggest saying "4 bytes" instead of just "4"

::: toolkit/components/url-classifier/ProtocolParser.cpp:844
(Diff revision 2)
> +    size_t numOfFixedLengthPrefixes = prefixes.size() / 4;
> +    PARSER_LOG(("* Raw addition (4)"));
> +    PARSER_LOG(("  - # of prefixes: %d", numOfFixedLengthPrefixes));
> +    PARSER_LOG(("  - Memory address: 0x%p", fixedLengthPrefixes));
> +  } else {
> +    // TODO: Process variable length prefixes including full hashes.

Do we have a bug number for this?

::: toolkit/components/url-classifier/ProtocolParser.cpp:845
(Diff revision 2)
> +    PARSER_LOG(("* Raw addition (4)"));
> +    PARSER_LOG(("  - # of prefixes: %d", numOfFixedLengthPrefixes));
> +    PARSER_LOG(("  - Memory address: 0x%p", fixedLengthPrefixes));
> +  } else {
> +    // TODO: Process variable length prefixes including full hashes.
> +    PARSER_LOG((" Raw addition (%d)", rawHashes.prefix_size()));

nit: again "%d bytes" instead of just "%d"

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:449
(Diff revision 2)
> -  mProtocolParser = new ProtocolParser();
> +  // See if all table names in |mUpdateTables| are all "-proto".
> +  bool useProtobuf = false;
> +  for (size_t i = 0; i < mUpdateTables.Length(); i++) {
> +    bool isCurProtobuf =
> +      StringEndsWith(mUpdateTables[i], NS_LITERAL_CSTRING("-proto"));
> +    if (useProtobuf && !isCurProtobuf) {

I think there's a bug here:

1. first element is `shavar`
2. second element is `proto`
3. third element is `proto`

Expected: we should see the warning and abort.

Actual: `useProtobuf` is false so the warning doesn't show and we keep going.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:450
(Diff revision 2)
> +  bool useProtobuf = false;
> +  for (size_t i = 0; i < mUpdateTables.Length(); i++) {
> +    bool isCurProtobuf =
> +      StringEndsWith(mUpdateTables[i], NS_LITERAL_CSTRING("-proto"));
> +    if (useProtobuf && !isCurProtobuf) {
> +      NS_WARNING("mUpdateTables should all or none end with '-proto'. "

grammar: I would suggest this instead:

    "Cannot mix 'proto' tables with other types within the same provider."
    
No need to hardcode the name of the provider here since there could be other V4 providers in the future.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:529
(Diff revision 2)
>    NS_ENSURE_STATE(mInStream);
>    NS_ENSURE_STATE(mUpdateObserver);
>  
>    mInStream = false;
>  
> +  // Required when the update is via protobuf since protobuf cannot be

nit: I don't think we need the comment since we have it elsewhere

::: toolkit/components/url-classifier/nsUrlClassifierUtils.h:47
(Diff revision 2)
>  
>  
>  public:
>    nsUrlClassifierUtils();
>  
> -  NS_DECL_ISUPPORTS
> +  NS_DECL_THREADSAFE_ISUPPORTS

Is that an accidental unrelated change or do we need to change this?

Comment 15

[Henry Chang [:hchang]]

https://reviewboard.mozilla.org/r/65182/#review64686

> Maybe we should ignore (with a warning) unknown treat types instead of aborting the whole process?
> 
> I would expect `return NS_OK;` so that we keep looking at the other list updates but ignore this one.

What if we still return an error but the for-loop (in ProtocolParserProtobuf::End()) doesn't break?

> Is this warning necessary or can we assume that if the response type isn't present then it's not a full update?

I don't know if we can do any assumption here. Maybe I have to re-read the spec or ask google in the end.

> I think there's a bug here:
> 
> 1. first element is `shavar`
> 2. second element is `proto`
> 3. third element is `proto`
> 
> Expected: we should see the warning and abort.
> 
> Actual: `useProtobuf` is false so the warning doesn't show and we keep going.

Thanks for finding the bug! I might change to use the first suffix to determine if the subsequent table name should or should not end with "-proto"

> Is that an accidental unrelated change or do we need to change this?

I had to make it thread safe because nsIUrlClassifierUtil would be called in ProtocolParserProtobuf::ProcessOneResponse, which would be running in non-main thread.

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65182/diff/2-3/

Comment 17

[François Marier [:francois]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

https://reviewboard.mozilla.org/r/65182/#review64928

That looks good to me.

Flagging gcp for a review to make sure it's OK to mark this as thread safe.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:451
(Diff revisions 2 - 3)
>    for (size_t i = 0; i < mUpdateTables.Length(); i++) {
>      bool isCurProtobuf =
>        StringEndsWith(mUpdateTables[i], NS_LITERAL_CSTRING("-proto"));
> -    if (useProtobuf && !isCurProtobuf) {
> -      NS_WARNING("mUpdateTables should all or none end with '-proto'. "
> -                 "Check pref 'browser.safebrowsing.provider.google4.lists'");
> +
> +    if (0 == i) {
> +      // Use the first table name to decice if all the subsequent tables

typo: decice

Comment 18

[François Marier [:francois]]

Marking this bug as "Do not land" to ensure it doesn't accidentally get pulled into 50. We should land it in 51.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

https://reviewboard.mozilla.org/r/65182/#review66054

Don't see any issues with using those utils from another thread. There's just some read-only data initialized at Init().

::: toolkit/components/url-classifier/ProtocolParser.h:132
(Diff revision 3)
> +  typedef google::protobuf::RepeatedPtrField<ThreatEntrySet> ThreatEntrySetList;
> +
> +public:
> +  ProtocolParserProtobuf();
> +
> +  virtual nsresult AppendStream(const nsACString& aData);

Add override annotations here. (Or make ProtocolParser an abstract base, which looks cleaner to me)

Comment 20

[Henry Chang [:hchang]]

According to [1], m-c has been merged to aurora. I am supposed to be able to remove the [DO NOT LAND] as Bug 1274112.

[1] http://hg.mozilla.org/releases/mozilla-aurora/graph/76df785c76c9

Comment 21

[Henry Chang [:hchang]]

Comment on attachment 8772351 [details]
Bug 1276595 - Use protobuf API to parse v4 update response.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65182/diff/3-4/

Comment 22

[Henry Chang [:hchang]]

Merge to Bug 1274112 to make try build and landing more convenient.