656510 - 500 Internal Server errors by fuzzing /search

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Add-on Builder, defect, P1)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Found these with PowerFuzzer (http://www.powerfuzzer.com/)

500 HTTP Error code with Vulnerable URL: https://builder.addons.mozilla.org/search/?q=a%29%3Benv

500 HTTP Error code with Vulnerable URL: https://builder.addons.mozilla.org/search/?q=%BF%27%22%28

500 HTTP Error code with Vulnerable URL: https://builder.addons.mozilla.org/search/?q=<script>var+pf_68747470733a2f2f6275696c6465722e6164646f6e732e6d6f7a696c6c612e6f72672f7365617263682f_71=new+Boolean();</script>

500 HTTP Error code with Vulnerable URL:
https://builder.addons.mozilla.org/search/addon/?q=a%29%3Benv

500 HTTP Error code with Vulnerable URL: https://builder.addons.mozilla.org/search/addon/?q=%BF%27%22%28

500 HTTP Error code with Vulnerable URL:
https://builder.addons.mozilla.org/search/addon/?q=<script>var+pf_68747470733a2f2f6275696c6465722e6164646f6e732e6d6f7a696c6c612e6f72672f7365617263682f6164646f6e2f_71=new+Boolean();</script>

500 HTTP Error code with Vulnerable URL:
https://builder.addons.mozilla.org/search/library/?q=a%29%3Benv

500 HTTP Error code with Vulnerable URL:
https://builder.addons.mozilla.org/search/library/?q=%BF%27%22%28

500 HTTP Error code with Vulnerable URL:
https://builder.addons.mozilla.org/search/library/?q=<script>var+pf_68747470733a2f2f6275696c6465722e6164646f6e732e6d6f7a696c6c612e6f72672f7365617263682f6c6962726172792f_71=new+Boolean();</script>

Comment 1

[Sean McArthur [:seanmonstar]]

The search term now gets all characters besides letters, numbers, and spaces stripped.

in master https://github.com/mozilla/FlightDeck/commit/036393be4c7f2be28937726774ee7ff62ad04179

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Reopening; while the 500s are fixed, https://builder-addons.allizom.org/search/?q=moo-panel is broken now, because the "-" char is stripped.

Dan says underscores "_" should probably also be allowed.

Comment 3

[Wil Clouser [:clouserw]]

That also stops people from searching in languages that don't use A-Za-z.

Comment 4

[Sean McArthur [:seanmonstar]]

I'm sure many places in FlightDeck have poor support for non-latin characters.

What should the expected behavior be, then? All the 500s are because ElasticSearch has trouble parsing certain characters. Will ElasticSearch accept characters from other languages?

Besides the possibility of placing dangerous HTML back into the results page, is there any other concern? Could we simply let Django escape the output?

Comment 5

[Wil Clouser [:clouserw]]

(In reply to comment #4)
> I'm sure many places in FlightDeck have poor support for non-latin
> characters.
>
> What should the expected behavior be, then? All the 500s are because
> ElasticSearch has trouble parsing certain characters. Will ElasticSearch
> accept characters from other languages?

Heh, it better.  We may need Dave's help enabling it.

> Besides the possibility of placing dangerous HTML back into the results
> page, is there any other concern? Could we simply let Django escape the
> output?

Django should escape everything on output, regardless of where it comes from.  I'm not sure what the concern is here.

Comment 6

[Sean McArthur [:seanmonstar]]

(In reply to comment #5)
> Django should escape everything on output, regardless of where it comes
> from.  I'm not sure what the concern is here.

No concern. I know that's what Django does. I'm saying that besides that, the only issue is ElasticSearch barfing on various non-alpha-numeric characters.

Comment 7

[Wil Clouser [:clouserw]]

Yep, so, we should give this bug to Dave Dash to fix and when he does he can back out your patch.  Thanks for writing it though

Comment 8

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

I'll try to help dive into this.  I am pressed for time, since I have today and tomorrow and then I'm off until the first week in June.

A branch of Input has no issues with unicode, so I'm sure there's some other stupid problem that we're not seeing.

If sean has time, I'd replace the existing search code with ElasticUtils' S object.  But we should also see if we can display the native errors.  I'll try to enable some debugging into elasticutils.

Comment 9

[Daniel Buchner [:dbuc]]

Two options:

1) If Dave can flip a switch in ElasticSearch that allows for the indexing and search of non-English characters then we can do that, but it would need to land before Friday for testing.

2) If there is not *very* easy and low risk, I'd rather reduce the deltas and launch the beta with English only support. We can make it a priority to enable non-English characters in the following quarter.

Comment 10

[Wil Clouser [:clouserw]]

We should do #2, that's why I moved this to the next milestone.  With sean's patch we effectively have only English support.  (If you want to add dashes or whatever, that can be done).

Comment 11

[Sean McArthur [:seanmonstar]]

An example of an ElasticSearch error for the content: http://pastebin.mozilla.org/1229345

Comment 12

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

This is done in a branch:

/commit/b243f28


I took all of stephen's examples and they seemed to be fine.

Blocking on 656394 since I built off that branch, but if we need to get this out sooner, I can do some grunt work to decouple it.

Comment 13

[Dave Dash [:davedash, :dd] (assign all bugs to mbrandt)]

/commit/85e6d55

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on https://builder-addons.allizom.org/search/, using PowerFuzzer.  Thanks!