[CONTACTS] HTML Tag & Java Script Injection Issue

VERIFIED FIXED in B2G C3 (12dec-1jan)

Status

P1
normal
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: wachen, Assigned: alberto.pastor)

Tracking

({sec-high})

unspecified
B2G C3 (12dec-1jan)
All
Gonk (Firefox OS)
sec-high

Firefox Tracking Flags

(blocking-basecamp:+, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
*Environment:
Phone - Otoro
Gaia Master(git) - a67126c5cf366cc2d635f7f5433a8363a3a93df4
Gecko Aurora(hg) - 66bada6ea6b2

*How to reproduce:
1. Unlock the phone
2. Go to contacts
3. create a contact
4. input some different combination of html tag and javascript tag

Some Tested Example Provided:
   a. <b>name</b>, the name would be bold font.
   b. <script>alert("a");</script> would be filtered out
   c. </html> would make u see nothing but you can still select it
   d. </input> would make no entry for people to enter
   e. HTML escape character injection: if you do "&lt;html&gt;", you can see <html> in list

I also expected if the input area is long enough, I can hide all other different tag and stuffs. I haven't tested all the app, but I would expect html5/css3/javascript OS to have serious issue about this on security.

*Expected Result:
   Should either do it in text or filter tags out.

*Actual Result:
   Depending on different tags, there are different results.
(Reporter)

Updated

6 years ago
blocking-basecamp: --- → ?

Comment 1

6 years ago
This is security related. It should be a blocking-basecamp:+

Updated

6 years ago
Group: core-security

Updated

6 years ago
blocking-basecamp: ? → +

Updated

6 years ago
Component: General → Gaia
from bug 809247 comment 0
  
   With support for importing vcard files (.vcf) or from services like Facebook
   there is potential to exploit this without physical access to the device.

Calling it sec-moderate if scripts are correctly being filtered out, but has anyone tried on* event handlers? If there's scripting of any kind this is probably sec-high
Keywords: sec-moderate
Created attachment 679321 [details]
Screenshot of onload attribute setting the background color

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Calling it sec-moderate if scripts are correctly being filtered out, but has
> anyone tried on* event handlers? If there's scripting of any kind this is
> probably sec-high

on* event handlers are being evaluated:
<img src="https://www.mozilla.org/favicon.ico" onload="this.style.backgroundColor='blue'">
changes the image background to blue.
Keywords: sec-moderate → sec-high
Assignee: nobody → anygregor
Applying the CSP should prevent the JS injections in on* event handlers.
(In reply to Fabrice Desré [:fabrice] from comment #5)
> Applying the CSP should prevent the JS injections in on* event handlers.

Sure, but we still shouldn't be parsing the HTML as it will cause problems when <>& are used in the fields.
Status: NEW → ASSIGNED
I will try to sanitize our contact fields before I put them in the DB.
(Reporter)

Comment 8

6 years ago
Just serve as a sidenote:
About CSP, there might still be minor issues after going with CSP.
http://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-1.0-specification.html
Like A server transmits its security policy for a particular protected resource as a collection of directives, such as default-src 'self'...

You can still inject <base ...> to finish the injection. (injection step for cross site attacking) So, please still be careful about handling all the tags if CSP applied.
(In reply to Gregor Wagner [:gwagner] from comment #7)
> I will try to sanitize our contact fields before I put them in the DB.

This is generally the opposite approach of what is recommended by OWASP[1] for a few reasons:
1) The contact database shouldn't be tied to a single output format (HTML in this case).  Suppose you want to implement json or vcard export: you wouldn't want to have to decode the HTML entities for that.
2) It will be a pain to alter the sanitization function once you already have contacts in the database because you would have to somehow migrate the old santized values to the new version.
3) Trusting what's in the database, by outputting it and assuming it's already been escaped is error prone and therefore may lead to security problem. e.g. importers from
the SIM card, Facebook, etc. would all need to properly escape the HTML.

It's safer to escape on output with an escapeHTML function already in Gaia.

[1] http://stackoverflow.com/questions/2060923/sanitize-html-before-storing-in-the-db-or-before-rendering-antixss-library-in
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → All

Updated

6 years ago
Component: Gaia → Gaia::Contacts

Updated

6 years ago
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Gregor what's the risk of this change? What's your ETA for a fix?
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
> 
> It's safer to escape on output with an escapeHTML function already in Gaia.
> 
> [1]
> http://stackoverflow.com/questions/2060923/sanitize-html-before-storing-in-
> the-db-or-before-rendering-antixss-library-in

That's fine with me.
alberto can you fix this?
(Assignee)

Updated

6 years ago
Assignee: anygregor → alberto.pastor
(Assignee)

Comment 12

6 years ago
Created attachment 688505 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6808

Pointer to Github pull-request
(Assignee)

Updated

6 years ago
Attachment #688505 - Flags: review?(anygregor)
Thanks for the quick fix Alberto!

Walter, Matthew can you run your test again with the pull request applied?
Flags: needinfo?(wachen)
(Reporter)

Comment 14

6 years ago
I ran my test again to see if it reproduce. I think it is fixed!
Flags: needinfo?(wachen)
Attachment #688505 - Flags: review?(anygregor) → review+
(Assignee)

Comment 15

6 years ago
https://github.com/mozilla-b2g/gaia/commit/347a8bdf941142cbe000f77513c790098200f614
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
status-firefox-esr10: --- → unaffected
status-firefox-esr17: --- → unaffected
(Reporter)

Comment 16

6 years ago
verified in 20121212 build http://releases.mozilla.com/b2g/
Status: RESOLVED → VERIFIED
status-b2g18: --- → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.