Closed Bug 1246301 Opened 4 years ago Closed 4 years ago

Incorrectly parsed value for update interval in shavar response


(Toolkit :: Safe Browsing, defect, P3)

44 Branch



Tracking Status
firefox48 --- fixed


(Reporter: mwobensmith, Assigned: dimi)



(Whiteboard: tpe-seceng)


(1 file, 1 obsolete file)

With the (malformed) shavar response:

n:9!=Ao[JDw+aN'|e}[ ]2


Firefox indicates an update success, with an update interval of 9 seconds (later corrected to 1800 seconds, since this is too short). The n field should be an integer, but the string is parsed and interpreted as an integer only until it reaches non-numeric characters


Reject invalid parameter for n value.

This is not a big issue by itself, just odd behavior.
Blocks: 1149867
Priority: -- → P3
Assignee: francois → nobody
We should treat this as a failed update.
Whiteboard: tpe-seceng
Assignee: nobody → dlee
Attached patch check update interval value (obsolete) — Splinter Review
Hi François,
You mentioned we should treat incorrect update interval as an update failure, but currently in ProtocolParser.cpp when n cannot be parsed successfully, we just set |mUpdateWait| to 0 [1].

I guess the behavior for incorrect value of n(too small or too large) and parse failure should sync, do you think we should return error or just set |mUpdateWait| to 0?

Attachment #8732695 - Flags: feedback?(francois)
Comment on attachment 8732695 [details] [diff] [review]
check update interval value

Review of attachment 8732695 [details] [diff] [review]:

If it works, my preference here would be to not touch the mUpdateWait unless the PR_sscanf() worked and then return an error code immediately (i.e. something else than NS_OK), aborting the parsing.

In terms of enforcing a minimum/maximum size, isn't this code sufficient?
Attachment #8732695 - Flags: feedback?(francois)
It turns out that the PR_sscanf() doesn't fail when it gets "9!=Ao[JDw+aN'|e}[ ]2", it simply returns "9" which we fix up later. I think that's OK.

The only case that would be worth fixing IMHO is "n:foobar" which will cause PR_sscanf() to return an error. In that case, we should return an error immediately and abort the parsing.
Attachment #8732695 - Attachment is obsolete: true
Comment on attachment 8734599 [details]
MozReview Request: Bug 1246301 - Incorrectly parsed value for update interval in shavar response. r=francois
Attachment #8734599 - Flags: review?(francois) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.