Profile Image encoding with Validator.escape

[mleanos]

With the new security enhancement for thwarting xss attacks implemented with b9e3fd1, the Profile Image stored in the browsers User object is now encoded with the escaping characters. When loading the image from the browser's User object, the image cannot be found; due to image path containing the escape characters.

To reproduce..

1. Log in with a User
2. Do a hard refresh (CTRL+F5)

You'll notice that the image cannot be found, and when viewing the User object from the source of the page you'll see that the profileImageURL field is encoded with the escaping characters.

The fix that comes to mind first is to decode (or remove escaping characters) from the User object data when it's loaded into the Authentication service.

@lirantal Is this what you were referring to about the Angular changes that might need to be made?

Anyone have any additional thoughts, or ideas? One thing that might be helpful, is for someone else to run through some test cases & watch the behavior of the User object. I seem to be experiencing odd behavior with the data that's loaded into the browser object; see these comments #1119 (comment) & #1119 (comment)

[mleanos]

I've been working on a solution to this.. So far I've come up with using Angular Sanitize, in which case I modified the client Authentication service with this:

user: {
  displayName: $sanitize($window.user.displayName),
  provider: $sanitize($window.user.provider),
  username: $sanitize($window.user.username),
  created: $sanitize($window.user.created.toString()),
  roles: $window.user.roles,
  profileImageURL: $sanitize($window.user.profileImageURL),
  email: $sanitize($window.user.email),
  lastName: $sanitize($window.user.lastName),
  firstName: $sanitize($window.user.firstName)
}

Does this seem redundant, since we're using Validator on the back-end? I'm leaning toward just sanitizing the profileImageURL field; and perhaps using a filter rather than updating this service.

Any thoughts?

[lirantal]

@mleanos yeah it seems redundant.
I think what we should do is maybe not sanitize the profile image?

[mleanos]

@lirantal You mean don't sanitize it on the back-end with Validator? I think that's a good idea. I think we're just having an issue with this particular field because it's a path.

I can change the PR to just remove the validator.escape being applied to the profile image on the server.

[lirantal]

yeah that's what I mean, can you try it and see if there are any issues still? if not then go ahead and modify this PR to accommodate the backend validation.

we should check though where in the code the image is being set to make sure no one can falsely set it to some malicious data like xss.

[mleanos]

@lirantal I didn't notice any issues after removing the escape method from the profileImageUrl field. We should look at sanitizing the actual contents of the uploaded image file though; meaning the binary data. WDYT?

I'm thinking we could remove the escape method from the backend in this PR to get the bug handled, and then look at implementing something more robust for sanitizing the file.

I'm on Gitter most days/nights if you wanna have a chat about this there.

[0xNacho]

Hi There.
As an independent security researcher, I think sanitizing the actual contents of the uploaded image file is not necessary (obviously, if you only show the image with tag). Try to validate that the uploaded file is a real image. If it's not, don't worry because your web server shouldn't "execute" your image file as a script.

[mleanos]

@0xNacho Thank you for the feedback. I was thinking it could be over-kill to sanitize the binary data. However, this is a little out of the realm of my expertise.

So it appears that there isn't really much of a risk here with the profileImageUrl.

@lirantal WDYT? If you agree, then I can revert my changes in the PR & then just remove the validator.escape for the profileImageUrl here https://github.com/meanjs/mean/blob/master/modules/core/server/controllers/core.server.controller.js#L18 so we can get a fix out for this bug

[mleanos]

@lirantal I've updated the PR to just remove the escaping of the profileImageUrl field, based on this conversation.

[lirantal]

Thanks, I'll review later and merge.