1288833 - Ensure that full hashes received in updates aren't used before we call gethash on them

Status: RESOLVED WONTFIX

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

From https://developers.google.com/safe-browsing/v3/update-guide#Changes:

"Full-length hashes obtained in shavar add chunks must also be verified via a full-length hash request prior to showing a warning. As with hash prefixes, the response will include an expiration time that specifies how long it may be cached."

This means that when we receive a full hash as part of an update, we need to call gethash on the first hit so that we get a TTL for it.

Comment 1

[Dimi Lee [:dimi]]

Hi Francois
Henry and I think this will be a good bug for thomas to get familiar with SB. But M0 as target milestone will be a little bit difficult(end of this week, 7/31) and also for M0,M1 we don't care about "gethash".

So how do you think we change the milestone to M1 and let thomas to work on this ?

Comment 2

[Ethan Tseng [:ethan]]

I think it's fine to move this bug to milestone 1. :)

Comment 3

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #1)
> Henry and I think this will be a good bug for thomas to get familiar with
> SB. But M0 as target milestone will be a little bit difficult(end of this
> week, 7/31) and also for M0,M1 we don't care about "gethash".
> 
> So how do you think we change the milestone to M1 and let thomas to work on
> this ?

That's fine by me. We won't need it until we do lookups, but let's do it at the same time as M1 since we can land this separately and start using it in V2.

Comment 4

[Thomas Nguyen (:tnguyen)]

(In reply to François Marier [:francois] from comment #3)
> That's fine by me. We won't need it until we do lookups, but let's do it at
> the same time as M1 since we can land this separately and start using it in
> V2.
(In reply to François Marier [:francois] from comment #0)
> 
> This means that when we receive a full hash as part of an update, we need to
> call gethash on the first hit so that we get a TTL for it.


I am assuming that we are trying to resolve this bug to sync with v4 implementation, but still a little bit confused.  Firstly, afaik, v2 does not support cache lifetime in the response of gethash api in v3 [2].
v3 : BODY = CACHELIFETIME LF HASHENTRY* EOF
Well, ignore this difference and I assume that we still have to verify full-length hash in update response by sending a gethash (probably server requires us to confirm getting the correct full hash), there is still a clear conflict to v2 specs

"A URL matches a full-length hash obtained in an add chunk returned as a response to an HTTP Request for Data, provided that such hash has not been removed from the list (e.g. via a sub chunk), and further provided that the list has been successfully updated via an HTTP Request for Data (where the entire update was successfully processed) within the past 45 minutes from the time a warning is to be provided, or"

Apparently, we are doing this change which only supports for v3 (not included in v2 and v4) which is intended to be ignored in our plan.  
[1] https://developers.google.com/safe-browsing/v3/update-guide#HTTPResponseForHashes
[2] https://web.archive.org/web/20160422212049/https://developers.google.com/safe-browsing/developers_guide_v2#LookupsAgeOfDataUsage
Do you have any idea/advice? Thanks you.

Comment 5

[François Marier [:francois]]

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #4)
> Well, ignore this difference and I assume that we still have to verify
> full-length hash in update response by sending a gethash (probably server
> requires us to confirm getting the correct full hash), there is still a
> clear conflict to v2 specs
> 
> "A URL matches a full-length hash obtained in an add chunk returned as a
> response to an HTTP Request for Data, provided that such hash has not been
> removed from the list (e.g. via a sub chunk), and further provided that the
> list has been successfully updated via an HTTP Request for Data (where the
> entire update was successfully processed) within the past 45 minutes from
> the time a warning is to be provided, or"

You're right, it does seem like we are already doing the right thing with respect to the V2 spec (but not V3).

Dimi/Henry, do you also read this the same way as Thomas and I?

Comment 6

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #5)
> (In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #4)
> You're right, it does seem like we are already doing the right thing with
> respect to the V2 spec (but not V3).
> 
> Dimi/Henry, do you also read this the same way as Thomas and I?

Yes, according to the spec, ask gethash for completion is only for v3 and v4.

Comment 7

[François Marier [:francois]]

Ok, let's leave the V2 code alone.