Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsanitized input #3002

Open
PineappleSnackz opened this issue Oct 22, 2022 · 9 comments
Open

Unsanitized input #3002

PineappleSnackz opened this issue Oct 22, 2022 · 9 comments
Labels
bug Issue that describes a problem with a feature that doesn't work as expected.

Comments

@PineappleSnackz
Copy link

The profile name field on the website does not properly sanitize user input. For example, I was able to insert a picture as well as a JavaScript popup box into my profile name. This can allow for XSS attacks, which could lead to things such as sensitive information being stolen or the user's account being taken over.

The website also does not properly sanitize data input for the following fields: sentence reviews, sentence languages, profile languages, profile language levels, and birthday year. However, these issues are less severe since they appear to only allow for invalid data to be entered, and not malicious code execution. Inputting invalid data into these fields also requires the user to send an edited HTTP post request, which is less likely to happen than simply inputting data into the profile name field.

Steps To Reproduce:
Unsanitized name input

  1. Open "https://dev.tatoeba.org/user/edit_profile" while being logged in.
  2. In the "Name" field, insert some html code (for example <script>alert("hello")</script>) and then save.
  3. Now when someone visits your profile, your name will display whatever html code you inserted.

Unsanitized HTTP posts

  1. Perform an action on the website that sends a HTTP post request.
  2. Log and save the HTTP post request that is sent (for example, using the developer tools in your web browser).
  3. Edit the saved HTTP post request and change some of the values.
  4. Send the edited HTTP post request to the website again.

I am not a security expert or web developer, so my understanding of these issues and their potential risks may be inaccurate. There may be other input fields on the website that do not properly sanitize user input. This bug report only covers what I have found.

@PineappleSnackz PineappleSnackz added the bug Issue that describes a problem with a feature that doesn't work as expected. label Oct 22, 2022
Yorwba added a commit that referenced this issue Oct 22, 2022
@Yorwba
Copy link
Contributor

Yorwba commented Oct 22, 2022

@PineappleSnackz Thank you for your reports, although it would've been nice to contact us about the XSS issue in private to limit the risk of someone exploiting it.

@trang @Gillux I've pushed a branch that I'm pretty sure should fix the issue: d44456c but I can't test it right now, so maybe one of you can take a look.

@PineappleSnackz
Copy link
Author

I do apologize; I realized that I probably should've sent an email a few seconds after submitting.

@Yorwba
Copy link
Contributor

Yorwba commented Oct 23, 2022

The XSS fix is now live and seems to work.

I'm leaving the unsanitized HTTP posts for later since they seem to be limited to inserting garbage data and I think they don't enable attacks on other users of the website.

Yorwba added a commit that referenced this issue Oct 30, 2022
This fixes an XSS vulnerability.

Refs #3002
@trang
Copy link
Member

trang commented Oct 30, 2022

The fix has been deployed so I'll close this issue.

@PineappleSnackz, thanks again for reporting this :)

And @Yorwba, thanks for the quick fix!

@trang trang closed this as completed Oct 30, 2022
@Yorwba
Copy link
Contributor

Yorwba commented Oct 30, 2022

@trang I only fixed the security-critical aspects of this. It's still possible to insert invalid values (e.g. check out Pineapple's profile on dev), just not in a way that allows attacking users via XSS (I hope).

@Yorwba Yorwba reopened this Oct 30, 2022
@trang
Copy link
Member

trang commented Nov 3, 2022

@Yorwba I see, my bad, I closed because I thought it was enough to just fix the security issue for the scope of this issue.

Just my two cents regarding invalid values:

  • For the name in the profile, I think it's not too much of a big deal to let the users write whatever they want. Obviously, there shouldn't really be a case where someone will want or need to use for instance > or < in their name, but who knows... There could be a valid case for it, if for some reason they want to add a smiley like >_< next to their name. We don't really need to enforce what people write in there, or at least I can't find a strong reason why we should restrict certain characters for the name input. We can let people write whatever they want. So I think it's enough that we made sure that if someone tries to inject malicious code, it doesn't get executed.
  • As for unsanitized POST requests, we can't prevent that from happening, at least not on client-side. We have to expect that anything can be sent from client-side. We can only do our best to make sure to have the proper logic on server-side to prevent attacks, or to prevent bugs. If someone decides to manually forge a POST request to send garbage, but it doesn't do anything harmful, then we don't we need to bother with it, in my opinion.

@PineappleSnackz
Copy link
Author

The issue with profile names has already been resolved, the only issue was that it rendered html code before.

And while invalid data being entered isn't really a security issue, it is still an issue. It doesn't make sense that a sentence review with the value "100" is accepted by the server, when only -1 to 1 should be accepted, or that the language of a sentence can be set to "cats".

@trang
Copy link
Member

trang commented Nov 7, 2022

It doesn't make sense that a sentence review with the value "100" is accepted by the server, when only -1 to 1 should be accepted, or that the language of a sentence can be set to "cats".

Yes, totally agree.

I actually had a bit of a different understanding of what you meant by "does not properly sanitize data input". I thought your expectation was that we clean up the inputs by escaping or encoding special characters, or by removing unwanted characters.

But it looks like what you actually meant is what I would describe as lack of proper validation and lack of proper error handling as well. In which case it would be good to align on what is the expected behavior for each of the inputs that were listed.

  • For the sentence review, we should indeed only save a review that is between -1 and 1, and not save if the value is outside of this range.
  • For the sentence language, I don't think it's possible to set the language of a sentence with an invalid language code because there is a validation rule for this. Perhaps you can elaborate what you did it exactly, @PineappleSnackz?
  • For the profile language, we should indeed not save a profile language with an invalid language code.
  • For the profile language level, I think we can still go ahead and save if we receive an invalid number, but we consider that anything that is not between 0 and 5 is "Unspecified".
  • For the birthday year, the UI only allows to set a year that is up to 100 years ago but I don't think we need to enforce it. If someone wants to set the year to 1779, I don't see too much of a problem. It could happen for instance if they want to create a profile for a fictional character that was born centuries ago.

@PineappleSnackz
Copy link
Author

Thanks for clearing it up!
As for the existing validation rules, they only seem to apply when adding a new sentence or translation, not when changing the language of an existing sentence.

trang added a commit that referenced this issue Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue that describes a problem with a feature that doesn't work as expected.
Projects
None yet
Development

No branches or pull requests

3 participants