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

RESOLVED FIXED

Status

Webmaker
Login
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: humph, Assigned: sedge)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 805738 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/161
Attachment #805738 - Flags: review?(kieran.sedgwick)
Attachment #805738 - Flags: review?(cade)
(Reporter)

Comment 2

4 years ago
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/hloflax6@email.com 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=hloflax7@email.com 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=hloflax8@email.com 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 - "-" "-"

Updated

4 years ago
Duplicate of this bug: 916896
(Assignee)

Comment 4

4 years ago
Created attachment 806032 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/162

Updated patch based on Dave's work.
Assignee: nobody → kieran.sedgwick
Attachment #805738 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #805738 - Flags: review?(kieran.sedgwick)
Attachment #805738 - Flags: review?(cade)
Attachment #806032 - Flags: review?(jon)
Attachment #806032 - Flags: review?(cade)
Comment on attachment 806032 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/162

r+
Attachment #806032 - Flags: review?(jon)
Attachment #806032 - Flags: review?(cade)
Attachment #806032 - Flags: review+
(Assignee)

Comment 6

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

You too please!
Attachment #806032 - Flags: review?(jon)

Updated

4 years ago
Attachment #806032 - Flags: review?(jon) → review+

Comment 7

4 years ago
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'

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 8

4 years ago
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

4 years ago
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

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 10

4 years ago
Created attachment 806773 [details]
https://bugzilla.mozilla.org/show_bug.cgi?id=917080

Sorry, I squashed it down.  Look for my comment in the PR
Attachment #806773 - Flags: review?(jon)
Comment on attachment 806773 [details]
https://bugzilla.mozilla.org/show_bug.cgi?id=917080

This is not the attachment I'm looking for :)
Attachment #806773 - Flags: review?(jon)
(Assignee)

Comment 12

4 years ago
Created attachment 807193 [details] [review]
https://github.com/mozilla/login.webmaker.org/pull/163

Hah. Hahaha. Oh Sedge...
Attachment #806032 - Attachment is obsolete: true
Attachment #806773 - Attachment is obsolete: true
Attachment #807193 - Flags: review?(jon)

Updated

4 years ago
Attachment #807193 - Flags: review?(jon) → review+

Comment 13

4 years ago
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

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain text/plain text/plain → text/x-github-pull-request text/x-github-pull-request text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.