Closed Bug 808946 Opened 12 years ago Closed 12 years ago

[CONTACTS] HTML Tag & Java Script Injection Issue

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Product:
Firefox OS Graveyard
Component:
Gaia::Contacts
Platform:
All
Gonk (Firefox OS)
Type:
defect

Tracking

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

Status:
VERIFIED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- fixed

People

(Reporter: wachen, Assigned: alberto.pastor)

References

Details

(Keywords: sec-high)

Attachments

(2 files)

Screenshot of onload attribute setting the background color
62.81 KB, image/png
Details
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6808
355 bytes, text/html
gwagner
: review+
Details 
*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.
blocking-basecamp: --- → ?
This is security related. It should be a blocking-basecamp:+
Group: core-security
blocking-basecamp: ? → +
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
(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.
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
Component: Gaia → Gaia::Contacts
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: anygregor → alberto.pastor
Pointer to Github pull-request
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)
I ran my test again to see if it reproduce. I think it is fixed!
Flags: needinfo?(wachen)
Attachment #688505 - Flags: review?(anygregor) → review+
https://github.com/mozilla-b2g/gaia/commit/347a8bdf941142cbe000f77513c790098200f614
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified in 20121212 build http://releases.mozilla.com/b2g/
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: