Security Flaw with localStorage
sarj21 opened this issue · 4 comments
There is a major security issue with how the user data is stored and accessed.
First off, in Session.js when a new Session object is created the user is stored in local storage
this.create = function(token, user){
...
$window.localStorage.currentUser = JSON.stringify(user);
...
};
This seems fine as they are themselves, however, in routes.js
if (requireAdmin && !Session.getUser().admin) {
return transition.router.stateService.target("app.dashboard");
}
if (requireVerified && !Session.getUser().verified) {
return transition.router.stateService.target("app.dashboard");
}
if (requireAdmitted && !Session.getUser().status.admitted) {
return transition.router.stateService.target("app.dashboard");
}
We get from Session.getUser which just fetches the user from localStorage. So, if a user just opens the inspector and changes the localStorage object:
> localStorage.getItem("currentUser")
< // the currentUser object
> localStorage.setItem("currentUser", {...,'admin': true,...}) // copied from above
they can gain access to the admin tab
We actually had an attendee of our hackathon note this to us and we are mostly admins already so we didn't recreate exactly before changing this. Either way, I made a change that makes Session.getUser use the existing users/:id
route in the api and I still store the user id in local storage, as that seems relatively harmless
Yeah that's true, but ...
First to try this easily you can add a break-point in the build app.js after it was loaded
this.getUser = function(){
return JSON.parse($window.localStorage.currentUser);
};
and then in the inspector you run these commands
t=JSON.parse($window.localStorage.currentUser)
t.admin=true
$window.localStorage.currentUser=JSON.stringify(t)
This will show you the admin view but of course no info will show in the screen as all of it is stored in the database and the if in the route you added 'isAdmin' function
router.get("/users/stats", isAdmin, function(req, res) {
UserController.getStats(defaultResponse(req, res));
});
and in 'isAdmin' the verification is being done using the user's token
function isAdmin(req, res, next) {
var token = getToken(req);
UserController.getByToken(token, function(err, user) { // from DB
if (err) {
return res.status(500).send(err);
}
if (user && user.admin) { // verification
req.user = user;
return next();
}
return res.status(401).send({
message: "Get outta here, punk!"
});
});
}
So he won't get anything that you marked as require Admin in the API
I guess this should be changed so that the user won't even be able to see the admin views but it's not something major
Can you please share the modifications you did to your code to fix this?
Yeah I think I even mentioned to the team it wasn't that damaging as I suspected they really couldn't get any info so you are correct, the admin views are back end validated at some point.
Here's the commits:
vthacks-org@060a462
vthacks-org@5d23944
I made a quick change to the Session.js to remove any reference to current user:
this.create = function(token, user){
$window.localStorage.jwt = token;
$window.localStorage.userId = user._id;
$rootScope.currentUser = user;
};
this.destroy = function(onComplete){
delete $window.localStorage.jwt;
delete $window.localStorage.userId;
$rootScope.currentUser = null;
if (onComplete){
onComplete();
}
};
...
this.getUser = function(){
var http = $injector.get('$http');
return http.get('/api/users/' + $window.localStorage.userId);
};
and then on the routes.js checked if they had a token and if they did then checked the user against the api:
if (requireLogin && !Session.getToken()) {
event.preventDefault();
$state.go('login', null, {
location: 'replace',
});
} else {
Session.getUser().then( (res) => {
var user = res.data;
if (requireAdmin && !user.admin) {
event.preventDefault();
$state.go('app.dashboard');
}
if (requireVolunteerOrAdmin && !user.volunteer && !user.admin){
event.preventDefault();
$state.go('app.dashboard');
}
if (requireVerified && !user.verified){
event.preventDefault();
$state.go('app.dashboard');
}
if (requireAdmitted && !user.status.admitted) {
event.preventDefault();
$state.go('app.dashboard');
}
});
}
but this is a kind of janky hot fix I put in place while I looked into it. It does seem to work, though. I bet there's a better fix that might be a little more far reaching. The session should probably be tracked with a cookie instead of via localStorage but that might take a bit more work, if we end up going that route I will happily change it and submit it.
TLDR hiding an interface from users of a client-side app requires never delivering the interface, but deliving the admin interface probably isn't harmful
Currently, hiding the admin interface (just the UI, not admin facing content such as user info) from users is not a feature in Quill. All Angular templates are accessible by the client, regardless of admin status.
To get around the proposed fix, just add a breakpoint after the API call or intercept the request itself.
If hiding the admin templates/frontend logic is super important, I'd recommend building a separate angular app that does authentication before delivery. This is only necessary if the admin interface contains features you don't want users to be aware of.
I think intercepting the request might be a good idea.
I know, just in general, storing volatile things in local storage is not usually recommended (e.g. user data). Like we have said, they can't actually harm anything or do any damage so there is no actual security flaw.
In general, they shouldn't be able to access admin though and I think my fix does a good job at just not allowing that the way this person tried it. You can close this issue if the existing behavior is fine and since its not dangerous and as you said, angular templates are all accessible by the client.