968576 - [SECURITY] Dangerous control characters allowed in Bugzilla text

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Manish Goregaokar [:manishearth]]

Bugzilla, currently, does not block control characters from being entered in code blocks.

Since it's a bug tracking website, we will have people passing around bash scripts. And code to be entered into an editor.

Now, just take a look at the following bash script:

#Creative Commons Attribution-ShareAlike / Author Manishearth
echo something
#Do more things


It looks pretty harmless.

If you copy paste it into an editor (do NOT copy paste it into the terminal), it will run `rm -r /`, as the first line, while it *looks* like an innocuous comment, is really one of the easiest ways to lose most of your data on a Linux system. (This works on Windows too, though the exact implementation would be different)

This works because the first line contains a lot of ^H characters that mask the real meaning of the 

While I don't have a testcase for it, this would work for Vim too. Many users here use Vim, and using a combination of the ESC (^[) character, one could theoretically execute some shell command (using the Vim command `:!` to run shell commands). This can again be hidden away in a larger comment or line of code using ^H.

There is a case for fixing this in the browser itself. I am filing separate bugs for that.

Comment 1

[Manish Goregaokar [:manishearth]]

(somehoe submitted an older draft of the report)

Just a note: in the post where I said "It will run `rm -r /`", I meant that it will display `rm -r /`. It would have run the code had you copied it into the terminal prompt directly, which I hope you didn't.

And the testcase only works in a terminal-based editor, like nano or vim. Most GUI editors render or hide control characters. Of course, the *actual* target is the prompt itself, but for viewing purposes you need a terminal based thing.


As for the unfinished sentence about ^H, I'm saying that there are a lot of ^H characters which the terminal interprets as a backspace (which it should), and deletes stuff. By backspacing the right characters, I have reduced the harmless comment into a harmful command.

Comment 2

[Manish Goregaokar [:manishearth]]

Reported bug 968584, asking for this to be fixed in FireFox as a whole by rendering such characters. Not sure if it'll gain traction, rendering unprintable things sounds a bit strange to me.

Comment 3

[Manish Goregaokar [:manishearth]]

Which security flag should this get?

sec-moderate states "Client bugs that might have high or critical results but require the user perform unusual or complex actions to trigger." -- I don't know if copying code into Vim or bash on a bug tracker is "unusual", I do it quite often.

sec-high states "requiring no more than normal browsing actions." -- which this doesn't. It does, however, give the attacker the ability to execute pretty much anything on the local machine since a lot of stuff can be hidden in seemingly innocuous code. Especially if you know Perl.

Comment 4

[:glob ✱]

the "harmfullness" of characters is wholly dependant on where they are being pasted, making true sanitisation within bugzilla tricky.  it took me a while to reproduce this issue because my terminal emulator was stripping ^H when pasting.

we could strip all control chars when rendering the comment with s/(?![\t\r\n])[[:cntrl:]]//g

Comment 5

[Manish Goregaokar [:manishearth]]

(In reply to Byron Jones ‹:glob› from comment #4)
> the "harmfullness" of characters is wholly dependant on where they are being
> pasted, making true sanitisation within bugzilla tricky.  it took me a while
> to reproduce this issue because my terminal emulator was stripping ^H when
> pasting.


True. Though the immediately harmful (and easiest to abuse) ones are ^H and ESC. 


As far as I can tell this is standard behavior on Linux distros; control chars are very much a part of the system clipboard. Which makes sense in a way, because ^M (line break) is a control char as well, along with some other things which you *do* want to be able to copy.

I was able to reproduce this on gnome-terminal, xterm, and terminator on Ubuntu 13.04. I'm reasonably certain it will work on Fedora as well.

IIRC this works for PowerShell and cmd as well, though I don't know about PuTTY (will verify later)



> we could strip all control chars when rendering the comment with
> s/(?![\t\r\n])[[:cntrl:]]//g


That regex ought to work.

Comment 6

[Manish Goregaokar [:manishearth]]

As far as I can tell, we just have to add a regex match to line 247[1]


Though MDN probably has this issue too (and there is a lot more code to copy there), we should look into that.

 [1]: http://mxr.mozilla.org/bugzilla/source/process_bug.cgi#247

Comment 7

[:glob ✱]

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> As far as I can tell, we just have to add a regex match to line 247[1]

no, if we do do this, we should do it in the web template at display time, and not touch the data stored.

Comment 8

[Manish Goregaokar [:manishearth]]

Ah, that works too. Though there seems to be only one entry point for comments in the code, but multiple exits.

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: HTML testcase

Also hosted at http://manishearth.anapnea.net/testcontrol.html

Comment 10

[:glob ✱]

> Though there seems to be only one entry point for comments in the code.

your guess isn't accurate - web services can also add comments.  if we were to strip control char prior to being inserted into the database, the comment class would be the correct location, not at a boundary (we'd also need to detect and fix any existing comments via checksetup).

fixing it with a template filter addresses the issue at hand with minimal risk. an unaltered form of the comment will still be available via alternative calls (xml.cgi, rpc, rest, ..), and it means if the fix is incorrect (eg. utf8 issue), then no data will be lost.

> Created attachment 8371259 [details]
> HTML testcase

we should *not* be sanitising attachment content in any way.

Comment 11

[Manish Goregaokar [:manishearth]]

(In reply to Byron Jones ‹:glob› from comment #10)
> fixing it with a template filter addresses the issue at hand with minimal risk.

Makes sense. First time I'm looking at Bugzilla code, I'm getting lost :p

> > Created attachment 8371259 [details]
> > HTML testcase
> 
> we should *not* be sanitising attachment content in any way.

Of course. I just attached HTML testpages to both of the bugs because it's useful to have a place to copy this from.

Though I just realized that I already had a testcase in the question itself. Oops.

Comment 12

[Manish Goregaokar [:manishearth]]

Attachment: Patch for comments.html.tmpl

Patched it with a second FILTER.

Not tested, and I'm really not good with Perl.

Comment 13

[Manish Goregaokar [:manishearth]]

Are there any other places where comments are exposed in the templates? Doesn't look like it, but I'm not familiar with the code.

Comment 14

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #4)
> we could strip all control chars when rendering the comment with
> s/(?![\t\r\n])[[:cntrl:]]//g

I'm not sure what you want to do with (?![\t\r\n]), but http://perldoc.perl.org/perlre.html#Look-Around-Assertions says that:

"If you are looking for a "bar" that isn't preceded by a "foo", /(?!foo)bar/ will not do what you want."

So this doesn't look like the right regexp.

Comment 15

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #14)

> "If you are looking for a "bar" that isn't preceded by a "foo", /(?!foo)bar/
> will not do what you want."

But we're not looking for a "bar" that isn't preceded by a foo. We're looking for a "bar" ([[:cntrl:]) that isn't a "foo" (the three "special" control characters). From the same page:

> That's because the (?!foo) is just saying that the next thing cannot be "foo"--and it's not, it's a "bar", so "foobar" will match.

Here, we _do_ want the next thing to not be "foo". He's using a lookahead as a negative regex.

Another way to do it would be to use the regex `(?[ [[:cntrl:]] - [\t\r\n] ])`. I think.

Comment 16

[:glob ✱]

Comment on attachment 8371542 [details] [diff] [review]
Patch for comments.html.tmpl

we should also filter control characters from bugmail (both plain text and html), in order to provide some protection to email clients which don't filter.

please create a new filter in Bugzilla/Template.pm and use it in the bug/comments.html.tmpl and the email/bugmail.{txt,html}.tmpl files.

if you aren't sure how to do this, i'm happy to create the patch for you.

Comment 17

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #16)
> we should also filter control characters from bugmail

If an attacker wants to abuse an email client, he doesn't need Bugzilla to do this. Also, if you follow this path, you would have to filter *all* possible field values everywhere, because these characters could be in the bug summary, bug comment, username, status whiteboard, attachment name, attachment description, etc... And as a reviewer, I guess you do like me and copy and paste the bug summary and the username into your commit message, so these fields would have to be filtered too. Why not simply fix the HTML filter to remove these characters for us? We really don't need nor want another filter (else you could have things like [% foo FILTER js FILTER html FILTER bar %] and this is prone to errors).

Also, validators should automatically remove these control characters, e.g. by using clean_text() instead of trim(). This could be done in a separate bug.

Comment 18

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #17)
> (In reply to Byron Jones ‹:glob› from comment #16)

> Also, if you follow this path, you would have to filter *all*
> possible field values everywhere, because these characters could be in the
> bug summary, bug comment, username, status whiteboard, attachment name,
> attachment description, etc...

Technically no, the only place people copy code from are the comments/description. And possibly from patch diffs (which we probably don't want to touch).

The current patch fixes it for the description and comments.

You're right about the bugmail thing. Most mail clients effectively handle control chars anyway, and for one that doesn't, I could just spoof the mail (unless they're using signed mail)

Comment 19

[Frédéric Buclin]

(In reply to Manish Goregaokar [:manishearth] from comment #18)
> Technically no, the only place people copy code from are the
> comments/description.

Technically, yes, these fields can contain control characters! I doubt you know what the habits of other users are. And as I said in my previous comment, reviewers (I am one of them) copy and paste several data into the commit message. So the right place to fix this problem is the HTML filter itself.

Comment 20

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #19)
> And as I said in my previous
> comment, reviewers (I am one of them) copy and paste several data into the
> commit message. So the right place to fix this problem is the HTML filter
> itself.


Hah, good point. Alright, I'll look into the HTML filter later today.

Comment 21

[Gervase Markham [:gerv]]

(Testing that SecureMail is working for me...)

Gerv

Comment 22

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #17)
> Why not simply fix the HTML  filter to remove these characters for us?

Looks like there isn't any HTML filter being applied on output for the comments, I think the data is entity-ified on input.

http://mxr.mozilla.org/bugzilla/source/template/en/default/bug/comments.html.tmpl#209 


So FILTER html-ing this may end up with double encoding.

In addition, in some cases I see html_quote (FILTER html) being applied _with_ clean_text() (which trims out extra whitespace and control chars) and sometimes it isn't. Of course, I can still add a `=~ s/[\x00-\x1F\x7F]+/ /g` to the HTML filter, that shouldn't be a problem, but it's not being used everywhere the HTML filter is used.

So, what I can do is:

 - Update the HTML filter to strip control chars
 - Update the comment and comment email templates to strip control chars (using a different filter created in Template.pm)

I'm not sure if I can bring uniformity and have them all use the same filter as there seems to be a lot of nonuniformity in usage.

Any suggestions?

Comment 23

[Manish Goregaokar [:manishearth]]

Attachment: strip control chars in html filter, add new controlchars filter, apply to comments. and bugmail templates.

This patch moves the filter to a different method, and uses that method in comments.html.tmpl and bugmail.*.tmpl.


It also updates the html filter (FILTER html is aliased to html_quote from Util.pm) so that it strips out control characters.

Comment 24

[:glob ✱]

Comment on attachment 8373983 [details] [diff] [review]
strip control chars in html filter, add new controlchars filter, apply to comments. and bugmail templates.

Review of attachment 8373983 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for taking the time to work through this patch :)

> You're right about the bugmail thing. Most mail clients effectively handle
> control chars anyway, and for one that doesn't, I could just spoof the mail
> (unless they're using signed mail)

this whole bug is about protecting users from clients which don't do the right thing (in this case terminal emulators and web browsers).

::: Bugzilla/Template.pm
@@ +681,5 @@
>                  return encode_base64($data);
>              },
> +
> +            # Strips out control characters
> +            controlchars => sub {

i think that name should be clearer: like strip_control_chars

@@ +683,5 @@
> +
> +            # Strips out control characters
> +            controlchars => sub {
> +                my ($data) = @_;
> +                $data =~ s/[\x00-\x1F\x7F]+/ /g;

we shouldn't be removing \t \r or \n.

we should be _removing_ the characters, not replacing them with a space. this mirrors how they are currently displayed in the browsers.

for clarity please use :cntrl: not a character range.

::: Bugzilla/Util.pm
@@ +71,5 @@
>      $var =~ s/"/"/g;
>      # Obscure '@'.
>      $var =~ s/\@/\@/g;
> +    # Removing control characters, bug 968576
> +    $var =~ s/[\x00-\x1F\x7F]+/ /g;

no need to mention a bug number, source control's blame does that for us.
the same comments above apply here.

Comment 25

[Frédéric Buclin]

Comment on attachment 8373983 [details] [diff] [review]
strip control chars in html filter, add new controlchars filter, apply to comments. and bugmail templates.

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>-  [%- comment_text FILTER quoteUrls(bug, comment) -%]
>+  [%- comment_text FILTER quoteUrls(bug, comment) FILTER controlchars -%]

There is no need for a new filter. quoteUrls() already calls html_quote(), and html_quote() already removes control characters.



>=== modified file 'template/en/default/email/bugmail.html.tmpl'

>-        <pre>[% comment.body_full({ wrap => 1 }) FILTER quoteUrls(bug, comment, to_user) %]</pre>
>+        <pre>[% comment.body_full({ wrap => 1 }) FILTER quoteUrls(bug, comment, to_user) FILTER controlchars %]</pre>

Same here.


If we decide to also filter plain text emails, then you also have to fix flagmail.txt, which also displays the comment.



Not sure how you generated the patch (bzr?), but PatchReader is unable to parse it correctly because the file name and modification date in headers are separated by spaces instead of tabs.

Comment 26

[Manish Goregaokar [:manishearth]]

Attachment: Incorporate comment 24, 25

Alright, I think I fixed that.

@Frederic I generated it with bzr, however I did so on a separate server* and lazily copied the text over. This time I've scp'd it instead, the spaces should work now. Apologies for the inconvenience.


*University proxy was giving me issues with bzr and while I can co over http://, it was really slow. So I sshed out and did the editing.

Comment 27

[:glob ✱]

Comment on attachment 8374213 [details] [diff] [review]
Incorporate comment 24, 25

r=glob :)

Comment 28

[Manish Goregaokar [:manishearth]]

Just realized something -- do support multibyte characters on Bugzilla?

If so, this fix may break this. Some multibyte encodings might use the control char byterange.

As far as I can tell, this isn't the case; since bugzilla is utf8 which preserves all ascii (unless the user is allowed to choose an encoding?). Just checking.

Comment 29

[Daniel Veditz [:dveditz]]

Since this isn't an attack on bugzilla itself or browsers this really can't get more than a sec-low. The results could be catastrophic for a user if you convince them to cut and paste into a command line, but that is definitely not "normal browsing" or normal use of bugzilla.

Comment 30

[Frédéric Buclin]

(In reply to Manish Goregaokar [:manishearth] from comment #28)
> Just realized something -- do support multibyte characters on Bugzilla?

If the utf8 parameter is turned on, then the DB uses UTF8 as encoding. If the parameter is left off, then you have no guarantee what the encoding is.

(19:43:04) dveditz: unless the DB is guaranteed to be utf-8 then the regexp is going to fail
(19:43:35) LpSolit: if the utf8 parameter is turned on, then yes, the DB is utf8-encoded
(19:43:41) dveditz: You'd have to put a banner on bugzilla "Not for use in Asian languages"
(19:43:57) dveditz: if it's off this regexp is trouble


Maybe only run the regexp if Bugzilla->params->{utf8} is true?

Comment 31

[Manish Goregaokar [:manishearth]]

Attachment: Ignore non-utf8 encodings

This patch only does the substitution for utf8.

(Not obsoleting the first one since I'm not yet sure if we need this)

Comment 32

[Frédéric Buclin]

Comment on attachment 8375715 [details] [diff] [review]
Ignore non-utf8 encodings

>=== modified file 'Bugzilla/Template.pm'

>+                # Only run for utf8 to avoid issues with other multibyte encodings that may be reassigning meaning to ascii characters.

This line is too long.



>=== modified file 'Bugzilla/Util.pm'

>     $var =~ s/\@/\@/g;
>-

Nit: don't remove this newline, for readability.


>+        # Remove control characters if the encoding is utf8
>+        # Othr multibyte encodings may be using this range; so ignore if not utf8

Missing period at the end of sentences. Also s/Othr/Other/.

Comment 33

[Frédéric Buclin]

Note also that this patch doesn't apply to branches, so a backport will be needed.

Comment 34

[Manish Goregaokar [:manishearth]]

Attachment: Ignore non-utf8 encodings v2

Fixed.

Comment 35

[Frédéric Buclin]

That's the same patch.

Comment 36

[Manish Goregaokar [:manishearth]]

Attachment: Ignore non-utf8 encodings v2

*facepalm*

Got it this time, hopefully.

Comment 37

[Dave Miller [:justdave]]

(In reply to Frédéric Buclin from comment #30)
> (In reply to Manish Goregaokar [:manishearth] from comment #28)
> If the utf8 parameter is turned on, then the DB uses UTF8 as encoding. If
> the parameter is left off, then you have no guarantee what the encoding is.

I was somehow convinced that the current version forced UTF8... I thought that param went away after a few versions when we thought people had enough time to migrate.  But sure enough I still see it on bugzilla-tip.  I assume we're holding off on approval until we're ready to roll with the security advisory and whatnot.

Comment 38

[Frédéric Buclin]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #37)
> I was somehow convinced that the current version forced UTF8... I thought
> that param went away after a few versions when we thought people had enough
> time to migrate.  But sure enough I still see it on bugzilla-tip.

We don't enforce it yet, see bug 902395.


> we're holding off on approval until we're ready to roll with the security
> advisory and whatnot.

I don't think this bug needs a security advisory. It's more a security enhancement than a bug fix. What we need are backports, though.

Comment 40

[Frédéric Buclin]

@glob: see comment 30. I think attachment 8374213 [details] [diff] [review] should be r- in favor of attachment 8375744 [details] [diff] [review].

@Manish: we need backports for 4.x, see comment 33. Do you want to write the patches yourself or should we?

Comment 41

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #40)
> @glob: see comment 30. I think attachment 8374213 [details] [diff] [review]
> should be r- in favor of attachment 8375744 [details] [diff] [review].
> 
> @Manish: we need backports for 4.x, see comment 33. Do you want to write the
> patches yourself or should we?

I can do it if you have some pointers to where I can get the code for a specific release (tags?) and what I need to do with it. I assume I just need to download the older code and make the same changes, right?

Comment 42

[Frédéric Buclin]

bzr co bzr://bzr.mozilla.org/bugzilla/4.4

Same with 4.2 and 4.0 (if the patch for 4.4 doesn't apply there). :)

Comment 43

[Manish Goregaokar [:manishearth]]

(In reply to Frédéric Buclin from comment #42)
> bzr co bzr://bzr.mozilla.org/bugzilla/4.4
> 
> Same with 4.2 and 4.0 (if the patch for 4.4 doesn't apply there). :)

Looks like flagmail.txt.tmpl doesn't exist in 4.2. Should I just skip it?

Comment 44

[Manish Goregaokar [:manishearth]]

Never mind, figured it out.

Comment 45

[Frédéric Buclin]

(In reply to Manish Goregaokar [:manishearth] from comment #43)
> Looks like flagmail.txt.tmpl doesn't exist in 4.2. Should I just skip it?

It's named template/en/default/request/email.txt.tmpl

Comment 46

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.4

Comment 47

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.2

Comment 48

[Manish Goregaokar [:manishearth]]

The patch for 4.2 also applies to 4.0, however the file "bugmail.txt.tmpl" seems to be new. Looking in to it now.

Comment 49

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.0

I added the filter to the newchangedmail template for 4.0. Not sure if that was the right way to do it, but it looks like it applies to comment bodies.

Comment 50

[Manish Goregaokar [:manishearth]]

(That file was replaced with the newer bugmail templates in bug 65477)

Comment 51

[:glob ✱]

Comment on attachment 8375744 [details] [diff] [review]
Ignore non-utf8 encodings v2

r=glob

Comment 52

[Frédéric Buclin]

Comment on attachment 8381034 [details] [diff] [review]
Backport: 4.4

I will let glob review backports as he reviewed the original one.

Comment 53

[:glob ✱]

Comment on attachment 8381034 [details] [diff] [review]
Backport: 4.4

bugzilla 4.4 and earlier need to run on perl v5.8, which doesn't support the "state" function.

Comment 54

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.0

Comment 55

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.2

Comment 56

[Manish Goregaokar [:manishearth]]

Attachment: Backport: 4.4

Comment 57

[:glob ✱]

Comment on attachment 8381972 [details] [diff] [review]
Backport: 4.0

r=glob

Comment 58

[:glob ✱]

Comment on attachment 8381973 [details] [diff] [review]
Backport: 4.2

r=glob

Comment 59

[:glob ✱]

Comment on attachment 8381974 [details] [diff] [review]
Backport: 4.4

r=glob

Comment 60

[Manish Goregaokar [:manishearth]]

Security advisory draft:

Class:       Other
Versions:    All
Fixed In:    4.4.3, 4.2.8, 4.0.12
Description: Currently, bugzilla does not filter out control characters. These can be used by an 
             attacker within code in comments to execute arbitrary commands when pasted 
             into a terminal-based editor or a command prompt.
References:  https://bugzilla.mozilla.org/show_bug.cgi?id=968576

Comment 61

[Manish Goregaokar [:manishearth]]

Update to sec-adv draft: The versions should include 4.5.3

Comment 62

[Frédéric Buclin]

(In reply to Manish Goregaokar [:manishearth] from comment #61)
> Update to sec-adv draft: The versions should include 4.5.3

I will take care of the sec adv. This is not the single sec bug to be fixed.

Comment 63

[Manish Goregaokar [:manishearth]]

I know, there are two. Thanks :)

Comment 64

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   0e39097..58b92d3  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   2f385c1..d3d080f  4.4 -> 4.4

To ssh://[e-mail]/bugzilla/bugzilla.git
   e5daf57..6066ff3  4.2 -> 4.2

To ssh://[e-mail]/bugzilla/bugzilla.git
   bab49d4..ffb3248  4.0 -> 4.0

Comment 65

[Frédéric Buclin]

Security advisory sent.

Comment 66

[[:Aleksej]]

(In reply to Manish Goregaokar [:manishearth] from comment #2)
> Reported bug 968584, asking for this to be fixed in FireFox as a whole by
> rendering such characters. Not sure if it'll gain traction, rendering

bug 868846 comment 15