when updating a code build recipe json returned isn't escaped
Bug #740096 reported by
David
This bug affects 1 person
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Launchpad itself |
Fix Released
|
Critical
|
Ian Booth |
Bug Description
So if you have a code project like
https:/
and it has a build recipe like this:
# bzr-builder format 0.3 deb-version xx<input/
lp:~daveb/+junk/delete_me
or
# bzr-builder format 0.3 deb-version xx<input/
in the description -->
when you edit the description or the Recipe contents the page gets updated with the json resulting from the post request. This is not escaped so the above example renders as
xx<input onfocus=
. While this is not a serious bug, it is a potential xss vector requiring a user's involvement to trigger.
Related branches
lp:~wallyworld/launchpad/recipe-text-xss
- Brad Crittenden (community): Approve (code)
-
Diff: 83 lines (+40/-3)2 files modifiedlib/lp/app/javascript/client.js (+21/-3)
lib/lp/app/javascript/tests/test_lp_client.js (+19/-0)
Changed in launchpad: | |
status: | New → Triaged |
importance: | Undecided → Critical |
Changed in launchpad: | |
assignee: | nobody → Ian Booth (wallyworld) |
status: | Triaged → In Progress |
visibility: | private → public |
visibility: | public → private |
tags: |
added: bad-commit-12671 removed: qa-needstesting |
Changed in launchpad: | |
status: | Fix Committed → Fix Released |
visibility: | private → public |
To post a comment you must log in.
On Wed, Mar 23, 2011 at 5:11 AM, Ian Booth <email address hidden> wrote: /bugs.launchpad .net/launchpad/ +bug/740096 /onfocus= alert(1) >", this is bad data. If some d(self, field_name, field, detail= NORMAL_ DETAIL)
> Hi
>
> See bug: https:/
>
> A recipe page will render ok when first loaded even with "bad" data for
> the text fields. As described in the bug, if a text field contains
> something like "<input/
> inline editing is done, the page is updated and an xhr response with
> data for all the object's fields is received, then for each field,
>
> _unmarshallFiel
>
> is called. This in turn invokes the unmarshall(self, entry, value)
> method on the IFieldMarshaller implementation for each field. For text
> fields, a TextFieldMarshaller is used. This just returns the value
> though. It doesn't escape the text data. I think it should? I hacked in
> a call to cgi.escape() and it would have worked but it barfed when it
> tried to escape the lp cache data. I hacked around this and it worked.
>
> Soooo, should TextFieldMarshaller escape stuff when it unmarshalls?
Nope. Escaping should be done on the other end (when reading/displaying
the data) not when storing it.
A thought experiment: what if there were some other non-HTML-rendering
client, it might be impossible to escape all strings such that both it
and web browsers would be happy with the strings. Therefore the raw
strings should be stored and the client should escape the strings before
using them in potentially dangerous ways.
> so, we need a way to push stuff through without escaping eg lp cache
> data. If not, what other approach should be used to ensure stuff is
> properly escaped when it is rendered?
If we escape on the read side the above won't be a problem.
YUI includes HTML escaping functionality: developer. yahoo.com/ yui/3/api/ Escape. html
http://