Closed Bug 1011628 Opened 10 years ago Closed 10 years ago

SecReview: Pulse Guardian

Categories

(mozilla.org :: Security Assurance: Review Request, task)

Product:
mozilla.org
Component:
Security Assurance: Review Request
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: akachkach, Assigned: psiinon)

References

Details

Answers to the security assurance questions:

> 1)    Who is/are the point of contact(s) for this review?

mcote and me.

> 2)    Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):

 Pulse Guardian is a web app that lets users sign up for a consumer account that can be used to open queues on the Pulse message system ( http://pulse.mozilla.org/ ).
 It's main goal is using that information, via a daemon that runs on the same server as the web app, to monitor the users that open a queue on Pulse, and warn them them if one their queues reach a certain size (or delete the queues if they just get too big) as overgrowing queues currently cause the Pulse server to shutdown.
 Also, now everyone uses the same username and password for pulse, so we have no control over who consumes the messages.


> 3)    Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:

Wiki: https://wiki.mozilla.org/Auto-tools/Projects/Pulse/PulseGuardian
Source on github: https://github.com/mozilla/pulse-guardian

> 4)    Does this request block another bug? If so, please indicate the bug number

Not a specific bug, but this should solve all the automated bugs submitted by Nagios (example: Bug 999412)

> 5)    This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

We'd like to land this as soon as possible so more projects can safely use Pulse.

> 6)    To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?

It will support "taskcluster" which should move to Pulse once all the current bugs are cleared.

> 7)    Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)
>   a)      Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?
>   b)      Are there any portions of the project that interact with 3rd party services?
>   c)      Will your application/service collect user data? If so, please describe 

a) No.
b) No. (only interacting with Pulse)
c) Yes. Emails, usernames and (salted + hashed) passwords of the users. For now, we still need to store passwords in clear for users that haven't activated their account (because some actions executed when an account's email is verified need the password).



> 8)    If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):

A dev instance of Pulse Guardian is currently deployed at: https://pulse-guardian.paas.allizom.org and will be moving to pulse-dev.allizom.org
I'll update the bug when this is done.


> 9)    Desired Date of review (if known from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and whom to invite.

Not known.
Assignee: nobody → sbennetts
Blocks: 1015037
OS: Linux → All
Hardware: x86_64 → All
The dev instance was moved here: http://pulseguardian.paas.allizom.org/

For passwords that are currently unencrypted, we were thinking we could instead always create RabbitMQ users (when the user signs up password) but give them no permission, and only set the correct permissions after the account is activated. (and removing users that stayed unactivated for a while)
Could I get an ETA on this review?  Just tonight we ran into a situation that PulseGuardian is designed to deal with (evergrowing queue from an unknown app).
Flags: needinfo?(sbennetts)
Sorry, just got back from PTO.
I'll start reading up about it.
When would be a good time to have a chat about this review?
I'm based in the UK re timezones.
Flags: needinfo?(sbennetts)
Sooner the better!  I'm the project lead, available generally from 9-6 Eastern Time.  :akachkach is the main developer; he's on Pacific Time.  Ideally we would both be there, but if it's easier to coordinate we can do it just with me.
Just wanted to say that since we submitted this bug, we've been working on using Persona for user authentication, and a side-effect that also solved the sec issue we had (having to store passwords in clear before the user have activated his account). Also, now, no passwords (encrypted or not) are stored by the app.
This doesn't actually block pulseguardian-dev deployment; in fact, it benefits from it.
Any news on the staging environment?
Flags: needinfo?(akachkach)
The dev instance have been deployed a couple days ago: https://pulse-dev.allizom.org/
Flags: needinfo?(akachkach)
I've had an initial look at https://pulse-dev.allizom.org/ and my first question is - what is the password configuration used for?
The mechanism looks very 'dangerous' - you dont need to supply the previous password, its vulnerable to CSRF attacks, there are no complexity rules ("password" is accepted).

Right now I have no queues - how do I set those up? Are there any example ones I can configure? Can someone supply details of queues I shouldnt be able to access?
(In reply to Simon Bennetts [:psiinon] from comment #9)
> I've had an initial look at https://pulse-dev.allizom.org/ and my first
> question is - what is the password configuration used for?
> The mechanism looks very 'dangerous' - you dont need to supply the previous
> password, its vulnerable to CSRF attacks, there are no complexity rules
> ("password" is accepted).

There is currently no way to compare with the current password: the password is only stored in rabbitmq (hashed with a custom algorithm of theirs). I could store the "current password" (salted + hashed) if that helps us avoid a CSRF attack.

> Right now I have no queues - how do I set those up? Are there any example
> ones I can configure? Can someone supply details of queues I shouldnt be
> able to access?

Queues let you consume messages from the Pulse server. You can create some with the mozillapulse library.

From Pulse Guardian, you're only supposed to see and delete your queues, not other users' queues. (except if your acccount is "admin", which is activated "manually" (changing the DB) for a couple accounts.
See the "Quickstart" section of http://pulse.mozilla.org/.

For testing, I create a test publisher (since pulse isn't locked down yet) and a test consumer with a durable queue, call listen() on the consumer object, and then send a message with the test publisher and make sure it's received by the consumer. Then ctrl-c to abort the listen() call, and publish more messages to fill the queue, since nothing is consuming from it at that point.

I could write up some simple code for you if you like.
An updated version is now online on http://pulse-dev.allizom.org/ , with password confirmation to avoid CSRF.
Sorry for not replying before - I was presenting at 2 conferences last week, and before then I was preparing for them ;)
I must admit I'm not so keen on storing the (salted, hashed) password if its not _really_ necessary.
Another option to prevent CSRF attacks you can use randomly generated anti CSRF tokens.
I did spot some other minor issues - shall I raise bugs for these?
Flags: needinfo?(akachkach)
No worries! I was also working on another project, so it gave me time to do the requested changes :).

All things considered, storing passwords this way should be OK for our usecase (we might need to do other changes in the future that request to confirm the rabbitmq password).

For other issues, I guess we can just address them in this bug (it's a rather small project).
Flags: needinfo?(akachkach)
No problem - I didnt want to go over the top:)

The issues I've found so far:

No password complexity rules - I was able to set a password of 'password' ;) This is generally considered to be dangerous as users (nearly) always take the path of least resistance.

The 'session' cookie is set without the secure flag, which means that the cookie can be accessed via unencrypted connections.

X-Frame-Options header is not included in the HTTP response to protect against 'ClickJacking' attacks.

I was a bit concerned to see that the username permitted 'special' characters like <>=".
Thats dangerous as you have to make sure they are properly encoded everywhere they are used (both now and in the future;)
If you dont really need them then I'd recommend restricting the user field to alphanumerics.
However I can confirm that right now you have encoded them properly.
FYI I actually scripted the creating of new Persona accounts so that I could automate fuzzing the user field using Zest (https://developer.mozilla.org/en-US/docs/zest). And it was so effective I talked about it in my recent talks at the AppSec EU and BSides Manchester security conferences :D

I havnt had a chance to look at the queues yet, I'll do that asap.
Thanks for the feedback Simon! Didn't really know about the cookies secure flag or the "X-Frame-Options" HTTP header.

Fixed that secured cookie, will fix "special characters in the username". I wonder what we could do to fix that password complexity rule: just request a minimum length and numeric characters? (p4ssw0rd and password1 would still work)

Also, this tool will only be used by Mozilla developers (that write consumers for Pulse), and the user accounts don't give access to anything sensitive (worst case scenario: account hacked, the hacker deletes queues and making clients crash, but that's easily detected), so some may use "dummy" passwords on purpose.

@cyliang: Could you add the "X-Frame-Options" header to the Pulse Guardian Apache configuration? (https://developer.mozilla.org/en-US/docs/Web/HTTP/X-Frame-Options#Configuring_Apache)
Flags: needinfo?(cliang)
You should also verify that whatever allowable characters you settle on for the username and password are acceptable to RabbitMQ as well. :)
@akachkach: The header change you requested has been made:

Index: modules/webapp/files/pulse-dev/etc-httpd/domains/pulse-dev.allizom.org.conf
===================================================================
--- modules/webapp/files/pulse-dev/etc-httpd/domains/pulse-dev.allizom.org.conf	(revision 89908)
+++ modules/webapp/files/pulse-dev/etc-httpd/domains/pulse-dev.allizom.org.conf	(revision 89909)
@@ -12,6 +12,8 @@
         Satisfy All
     </FilesMatch>

+    Header always append X-Frame-Options SAMEORIGIN
+
     ## Note the hardcoded log destination path.
     LogFormat "%h %l %u %t \"%r\" %>s %b \"%{Referer}i\" \"%{User-Agent}i\"" combined
     ErrorLog "|/usr/sbin/rotatelogs /var/log/httpd/pulse.mozilla.org/error_log_%Y-%m-%d-%H 3600 -0"
Flags: needinfo?(cliang)
The dev instance was updated (bug 1033638), added restrictions to usernames and passwords, cookies should have the secure flag and we should have the X-Frame-Options HTTP header.

Could you please confirm the issues you raised were fixed, Simon?
Flags: needinfo?(sbennetts)
Yes, I can confirm that all of those issues are now fixed :)
Flags: needinfo?(sbennetts)
Is this review complete then?
I would like to test accessing / deleting other users queues, although only if thats relevant at this stage.
How can I get some queues added to test users in Pulse Guardian?
Right now my test users dont have any queues and it doesnt look like I can add them, which makes testing that part tricky ;)
Flags: needinfo?(mcote)
Following a discussion on irc it looks like the permission model changes are scheduled for a later phase, so theres no point testing them :)
In which case this review is complete.
Thanks for your help guys.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(mcote)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.