Loop Server Security Review
https://github.com/mozilla-services/loop-server/blob/master/loop/storage/redis.js#L57
Would it make sense to check if the call url already exists for another user? (I understand the call URLs need to be unique, but what if they are not for some reason?) -
https://github.com/mozilla-services/loop-server/blob/master/loop/storage/redis.js#L61
I think that else if should be just an if. Because we want to do both tests?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/storage/redis.js#L298
May need a check to prevent looping until the stack blows up. Or does ttl() always return a valid value guaranteed? What happens if callId does not exist?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/storage/redis.js#L317
For debugging it is probably better to log what the invalid state is, not what the allowed states are. (Which are static)
https://github.com/mozilla-services/loop-server/blob/master/loop/encrypt.js#L6
Are we confident that the Sodium library is good?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/encrypt.js#L20 https://github.com/mozilla-services/loop-server/blob/master/loop/encrypt.js#L41
Why just check the text parameter? Should also check if the passphrase is present and non-empty?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/hawk.js#L86
Does the Hawk middleware also check for replayed requests? If a request were replayed, could that be bad for a user?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L332
The validation of the simplePushURL is a bit simplistic. I think we can do a proper check. And I also wonder if we should pin down the allowed domains in the push urls. What if I supply some random URL, would the app push to that?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L407
If the storageStatus returns an error then we can skip doing the alive test of tokbox.
I am curious if the /__heartbeat__ endpoint is public. Since we call a third party service in it, with API credentials, we could be used to indirectly attack tokbox. Should be rate-limit this endpoint?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L450
Stricter input validation should be done in POST /register. With the current simplePushURL validation it is possible to post anything in there from just "http" to "httpverylongstringofmanykilobytesormaybeevenmore..."
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L652
For DELETE /call-url/:token we call validateToken. but validateToken() only checks if the token exists. Not if the current logged in user is allowed to actually delete it.
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L385
The req.body.callerID and req.body.issuer are taken as-is. Do these need to be validated or cleaned up?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L476
The documentation says "The callerId is supposed to be a valid email address" but I don't think we actually check that?
-
https://github.com/mozilla-services/loop-server/blob/master/loop/index.js#L668
I think this API call can be abused to do a HTTP request amplification attack. To do this I would call /register 10 times (1st time anonymous, then 9 times with the generated anonymous hawk credentials) with unique simplePushURLs that point to the attacked web site. (These can be slightly different, maybe it does not matter what the actual URL is). I would then call POST /calls/:token to initiate a call, which will result in 10 HTTP PUT requests to the site under attack.
If we know beforehand what the push urls look like, i think we want to whitelist them to prevent this kind of abuse.
