Closed Bug 1276595 Opened 6 years ago Closed 6 years ago

Parse SafeBrowsing v4 update response

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1274112

People

(Reporter: hchang, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(1 file, 8 obsolete files)

No description provided.
Assignee: nobody → hchang
Depends on: 1273412
Depends on: 1275198
Attached patch WIP Patch (obsolete) — Splinter Review
Attachment #8757853 - Attachment is obsolete: true
Attached patch Test case (obsolete) — Splinter Review
Attached patch Part 1: Parse update response (obsolete) — Splinter Review
Attachment #8757915 - Attachment is obsolete: true
Attached patch Part 2: Test case (obsolete) — Splinter Review
Attachment #8758524 - Attachment is obsolete: true
Whiteboard: #sbv4-m0
Attachment #8758593 - Attachment is obsolete: true
Attachment #8758594 - Attachment is obsolete: true
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 :)
Attachment #8771369 - Flags: feedback?(dlee)
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
Attachment #8771369 - Flags: feedback?(dlee)
Attachment #8771369 - Attachment is obsolete: true
Attachment #8772313 - Flags: feedback?(dlee)
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.
Attachment #8772313 - Flags: feedback?(dlee) → feedback+
Attached patch Bug1276595.patch (obsolete) — Splinter Review
No longer blocks: safebrowsingv4
Depends on: 1277488
No longer depends on: 1275198
Status: NEW → ASSIGNED
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/
Attachment #8772351 - Attachment description: Bug 1276595 - Use protobuf API to parse v4 update response. . → Bug 1276595 - Use protobuf API to parse v4 update response.
Attachment #8773599 - Attachment is obsolete: true
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?
Attachment #8772351 - Flags: review?(francois) → review-
QA Contact: mwobensmith
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 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/
Attachment #8772351 - Flags: review- → review?(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
Attachment #8772351 - Flags: review?(francois) → review+
Attachment #8772351 - Flags: review?(gpascutto)
Marking this bug as "Do not land" to ensure it doesn't accidentally get pulled into 50. We should land it in 51.
Summary: Parse SafeBrowsing v4 update response → [DO NOT LAND] Parse SafeBrowsing v4 update response
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)
Attachment #8772351 - Flags: review?(gpascutto) → review+
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
Summary: [DO NOT LAND] Parse SafeBrowsing v4 update response → Parse SafeBrowsing v4 update response
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/
Attachment #8772351 - Flags: review+
Merge to Bug 1274112 to make try build and landing more convenient.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1274112
Attachment #8772351 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.