917080 - Don't confuse params for static paths in user/ and username/ routes

Status: RESOLVED FIXED

(Webmaker Graveyard :: Login, defect)

Description

[David Humphrey (:humph)]

We had a case via the MakeAPI where someone was trying to get data from the server by doing bad things, specifically, trying to pass a username like this:

/user//../../../../../../../../../../../etc/passwd

Here, our Login code gets confused and thinks the extra /s mean it should look for a static route vs. use the entire string as a username.

What we should do is take everything after the /user/* or /username/* as the entire username string, and let our validation and such deal with it.  In express you do this by saying that a route is:

app.get( '/routename/*', ...)

Then, to get the value:

var username = req.params[ 0 ];

I've got a patch for this that I'll post in a sec, and I probably need someone to shepherd it in tomorrow, since I'll be afk most of the day.

Comment 1

[David Humphrey (:humph)]

Attachment: https://github.com/mozilla/login.webmaker.org/pull/161

Comment 2

[David Humphrey (:humph)]

Test runs shows we might still have some issues with this, see stack traces (not sure what those are about):

   ✓ should successfully return an account when attempting the retrieve an existing user by id 
    ◦ should error on attempting to retrieve a non-existant account: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /user/[e-mail] HTTP/1.1" 404 58 "-" "-"
    ✓ should error on attempting to retrieve a non-existant account 
    ◦ should deal with bogus usernames that look like routes: Error: Forbidden
    at SendStream.error (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:145:16)
    at SendStream.pipe (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:310:52)
    at Object.static (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/static.js:84:8)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.logger (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/logger.js:156:5)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.exports.allowCorsRequests [as handle] (/Users/dave/Sites/repos/login.webmaker.org/app/http/controllers/application.js:22:3)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.expressInit [as handle] (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/lib/middleware.js:31:5)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /user//../../../../../../../../../../../etc/passwd HTTP/1.1" 403 - "-" "-"
    ✓ should deal with bogus usernames that look like routes 
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/295 HTTP/1.1" 200 3 "-" "-"
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/296 HTTP/1.1" 200 3 "-" "-"
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/297 HTTP/1.1" 200 3 "-" "-"

  GET /isAdmin/:id
info: HTTP server listening on port 3000.
    ◦ should successfully return when attempting to check an existing user: Sending the welcome email is disabled
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "POST /user HTTP/1.1" 200 415 "-" "-"
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /isAdmin?id=[e-mail] HTTP/1.1" 200 21 "-" "-"
    ✓ should successfully return when attempting to check an existing user 
    ◦ should error on attempting to check a non-existant account: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "GET /isAdmin?id=[e-mail] HTTP/1.1" 404 78 "-" "-"
    ✓ should error on attempting to check a non-existant account 
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:37 GMT] "DELETE /user/298 HTTP/1.1" 200 3 "-" "-"

  GET /user/username/:userid
info: HTTP server listening on port 3000.
    ◦ should error if no userid is passed: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/ HTTP/1.1" 400 27 "-" "-"
    ✓ should error if no userid is passed 
    ◦ should return 200 if username in use: Sending the welcome email is disabled
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "POST /user HTTP/1.1" 200 416 "-" "-"
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/hloflax9 HTTP/1.1" 200 56 "-" "-"
    ✓ should return 200 if username in use 
    ◦ should return 403 if username is a badword: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/damn HTTP/1.1" 403 41 "-" "-"
    ✓ should return 403 if username is a badword 
    ◦ should return 404 if username is available: 127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username/hloflaxa HTTP/1.1" 404 43 "-" "-"
    ✓ should return 404 if username is available 
    ◦ should deal with bogus usernames that look like routes: Error: Forbidden
    at SendStream.error (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:145:16)
    at SendStream.pipe (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/send/lib/send.js:310:52)
    at Object.static (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/static.js:84:8)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.logger (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/logger.js:156:5)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.exports.allowCorsRequests [as handle] (/Users/dave/Sites/repos/login.webmaker.org/app/http/controllers/application.js:22:3)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
    at Object.expressInit [as handle] (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/lib/middleware.js:31:5)
    at next (/Users/dave/Sites/repos/login.webmaker.org/node_modules/express/node_modules/connect/lib/proto.js:190:15)
127.0.0.1 - - [Tue, 17 Sep 2013 01:16:38 GMT] "GET /user/username//../../../../../../../../../../../etc/passwd HTTP/1.1" 403 - "-" "-"

Comment 4

[Kieran Sedgwick (:sedge)]

Attachment: https://github.com/mozilla/login.webmaker.org/pull/162

Updated patch based on Dave's work.

Comment 5

[Chris DeCairos (:cade)]

Comment on attachment 806032 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/162

r+

Comment 6

[Kieran Sedgwick (:sedge)]

Comment on attachment 806032 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/162

You too please!

Comment 7

[[github robot]]

Commits pushed to master at https://github.com/mozilla/login.webmaker.org

https://github.com/mozilla/login.webmaker.org/commit/0e3114a20dfb97076ba98ac87f3ad4eaf8c8359d
Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes

https://github.com/mozilla/login.webmaker.org/commit/8748086e16a96589acaa5bb5027928095437f934
Merge branch 'bug917080'

Comment 8

[[github robot]]

Commit pushed to master at https://github.com/mozilla/login.webmaker.org

https://github.com/mozilla/login.webmaker.org/commit/bd9dcecb068eedaa615414674ef88d0477ccd4dc
Revert "Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes"

This reverts commit 0e3114a20dfb97076ba98ac87f3ad4eaf8c8359d.

Comment 9

[Jon Buckley [:jbuck]]

This patch prevents account deletions from occuring. I get the following error:

127.0.0.1 - - [Wed, 18 Sep 2013 16:45:16 GMT] "POST /account/delete HTTP/1.1" 500 - "http://localhost:9000/account" "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0"
TypeError: Cannot call method 'match' of undefined
    at parseQuery (/Users/jon/Sites/login.webmaker.org/app/models/user/sqlController.js:77:40)
    at Object.getUser (/Users/jon/Sites/login.webmaker.org/app/models/user/sqlController.js:140:27)
    at Object.getUser (/Users/jon/Sites/login.webmaker.org/app/models/user/index.js:17:17)
    at controller.del (/Users/jon/Sites/login.webmaker.org/app/http/controllers/user.js:63:16)
    at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37)
    at checkPersona (/Users/jon/Sites/login.webmaker.org/app/http/routes.js:82:7)
    at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37)
    at /Users/jon/Sites/login.webmaker.org/node_modules/express/node_modules/connect/lib/middleware/csrf.js:56:5
    at callbacks (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:161:37)
    at param (/Users/jon/Sites/login.webmaker.org/node_modules/express/lib/router/index.js:135:11)

Backing it out in the mean time

Comment 10

[Kieran Sedgwick (:sedge)]

Attachment: https://bugzilla.mozilla.org/show_bug.cgi?id=917080

Sorry, I squashed it down.  Look for my comment in the PR

Comment 11

[Jon Buckley [:jbuck]]

Comment on attachment 806773 [details]
https://bugzilla.mozilla.org/show_bug.cgi?id=917080

This is not the attachment I'm looking for :)

Comment 12

[Kieran Sedgwick (:sedge)]

Attachment: https://github.com/mozilla/login.webmaker.org/pull/163

Hah. Hahaha. Oh Sedge...

Comment 13

[[github robot]]

Commit pushed to master at https://github.com/mozilla/login.webmaker.org

https://github.com/mozilla/login.webmaker.org/commit/a92facc09acc3d7252d2802e1485f3b1ecd4c1d2
Fix Bug 917080 - Don't confuse params for static paths in user/ and username/ routes