[autoconfig][regression] Allow image data URLs

RESOLVED FIXED in Thunderbird 66.0

Status

defect
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: BenB, Assigned: BenB)

Tracking

({regression})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr6065+ fixed, thunderbird65 fixed, thunderbird66 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Bug 1514628 changed the icon URL from http: to data:image/png;base64... .
However, data: URLs are forbidden, so the addon doesn't show up at all.

data: URLs are highly dangerous in chrome code. They can contain anything, including javascript, e.g. data:text/javascript; and data:text/html; and similar. If these come from the network, and they are run from chrome code, they allow the attacker to run arbitrary code with system privileges, i.e. a remote code execution bug, a critical security bug. These are one of the most dangerous URLs in chrome. These should be avoided at all costs.

However, I guess that data:image/png; and data:image/jpeg; are fine, because they cannot contain JavaScript code.

So, this change opens this up a little bit, allowing specifically these image data: URLs, and only those.
OS: Unspecified → All
Hardware: Unspecified → All
Target Milestone: --- → Thunderbird 66.0
Version: unspecified → 66
Posted patch Fix, v1 (obsolete) — Splinter Review
Attachment #9032900 - Flags: review?(neil)
Comment on attachment 9032900 [details] [diff] [review]
Fix, v1

r=me with comment nit fixed as discussed.
Attachment #9032900 - Flags: review?(neil) → review+
Posted patch Fix, v2 (obsolete) — Splinter Review
Add comment to explain why not all data:image/ are OK, and SVG cannot be allowed.
Comment on attachment 9032926 [details] [diff] [review]
Fix, v2

Carry over Neil r+ from above.
Attachment #9032926 - Flags: review+
Keywords: checkin-needed
Attachment #9032900 - Attachment is obsolete: true
Posted patch Fix, v2, with hg header (obsolete) — Splinter Review
Attachment #9032926 - Attachment is obsolete: true
Attachment #9032933 - Flags: review+
// DANGER ZONE. data:text/javascript or data:text/html run in
              ^ How about a colon?

// the caller's security context and contain code and might allow
// arbitrary code execution, so these must be prevented at all costs.

Maybe you want to fix the English a bit here and clarify it. Usually a list only uses "and" once, and commas before that. Also, which code does text/html contain? HTML code? How about
... run in the caller's security context, could contain code and might allow arbitrary code execution, ...

Code-wise, all the checks are case sensitive, is that intended?
Umm, "NeilAway" is some strange IRC nick, wouldn't we just put r=Neil?
> Code-wise, all the checks are case sensitive, is that intended?

Yes.

Comment fixed.
Attachment #9032933 - Attachment is obsolete: true
Attachment #9032940 - Flags: review+
Pushed by mozilla.BenB@bucksch.org:
https://hg.mozilla.org/comm-central/rev/1f255495f7a1
[autoconfig] Allow PNG and JPEG image data: URLs. r=Neil
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #9032940 - Flags: approval-comm-beta+
Attachment #9032940 - Flags: approval-comm-esr60?
Comment on attachment 9032940 [details] [diff] [review]
Fix, v4, with hg header

Using Neil's patch from the try which is rebased.
Attachment #9032940 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.