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

XSS vulnerability everywhere #15

Closed
AlexDaniel opened this Issue Sep 1, 2015 · 14 comments

Comments

Projects
None yet
2 participants
@AlexDaniel

AlexDaniel commented Sep 1, 2015

Although taint checking is on, it seems like there are no checks at all.

Some examples (you have to make someone click this link):

  • ?do=admin;page=index<script>alert('badum-tss')</script>
  • ?do=admin<script>alert('badum-tss')</script>test (even though it errors out, javascript still runs)
  • possibly more…

This, however, does not let you to leave some malicious javascript on the page and then just sit back.

But this does:
screenshot with <b> injected into page name
(arbitrary html injected into page name. In this case, it is <b>)

Please note that I was actually aiming for write access vulnerability (mentioning it because it can be seen on the screenshot). Possibly problematic lines:

sub WriteDB {
  # We receive file name, and hash
  my $filename = shift;
  my %filedata = %{shift()};
  $filename =~ m/^(.*)$/; $filename = $1;
  open(FILE, ">$filename") or push @Messages, "WriteDB: Unable to write to $filename: $!";

It seems like taint checking has turned on its alarms on this code, but was just silenced off. The problem with arbitrary filenames is that you can pass any kind of stuff there, for example /../../somefile. This should work! Unfortunately (luckily), I was unable to get it to work, but it should be investigated. Basically the first character will be used as a path in $PageDir (let's say data/), which turns it into data///../../somefile – perfectly valid file path. I wonder why I couldn't get it to write the file…

(Sorry if you don't like such reports to be posted on GitHub. I see no problem in posting it here publicly. The whole thing is about poking <b> into various places for 30 minutes)

@ajgraves ajgraves added the bug label Sep 1, 2015

@ajgraves ajgraves self-assigned this Sep 1, 2015

@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 1, 2015

Maybe I should elaborate on the path in page name thing.

Here, some stuff is stripped: https://github.com/ajgraves/aneuch/blob/master/aneuch.pl#L172
But it does not matter. It seems like this code is not even related to DoPostingDiscuss which does this:

AppendPage(GetParam('file'), GetParam('text'), GetParam('uname'),
      GetParam('url'));

That's without any checks. There are no checks in GetParam too. AppendPage simply calls WriteDB, so a weird path with dots and slashes will actually pass all the way through. If you find what was breaking my perfect attempt to write into arbitrary path, please tell me, that's interesting.

AlexDaniel commented Sep 1, 2015

Maybe I should elaborate on the path in page name thing.

Here, some stuff is stripped: https://github.com/ajgraves/aneuch/blob/master/aneuch.pl#L172
But it does not matter. It seems like this code is not even related to DoPostingDiscuss which does this:

AppendPage(GetParam('file'), GetParam('text'), GetParam('uname'),
      GetParam('url'));

That's without any checks. There are no checks in GetParam too. AppendPage simply calls WriteDB, so a weird path with dots and slashes will actually pass all the way through. If you find what was breaking my perfect attempt to write into arbitrary path, please tell me, that's interesting.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 1, 2015

Owner

Thank you for the report! I will begin checking into this now.

Owner

ajgraves commented Sep 1, 2015

Thank you for the report! I will begin checking into this now.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 5, 2015

Owner

For documentation's sake:

Places where sanitization is needed:

  • DoPostingSpam
  • DoPostingEditing
  • DoPostingDiscuss
  • DoPostingUpload
  • DoPosting
  • WritePage
  • WriteDB
Owner

ajgraves commented Sep 5, 2015

For documentation's sake:

Places where sanitization is needed:

  • DoPostingSpam
  • DoPostingEditing
  • DoPostingDiscuss
  • DoPostingUpload
  • DoPosting
  • WritePage
  • WriteDB
@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 5, 2015

What about sanitizing it right in sub GetParam? Not that you should not do sanitization (other than HtmlQuote) in places you mentioned, but if you forget to sanitize something, at least it will be html-quoted. That's what Oddmuse does, for example.

AlexDaniel commented Sep 5, 2015

What about sanitizing it right in sub GetParam? Not that you should not do sanitization (other than HtmlQuote) in places you mentioned, but if you forget to sanitize something, at least it will be html-quoted. That's what Oddmuse does, for example.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 5, 2015

Owner

That thought occurred to me, especially because (from what I can tell right now) everything that needs sanitization is going to be calling GetParam for 'file'.

I have modified GetParam to call QuoteHTML before returning a value.

Owner

ajgraves commented Sep 5, 2015

That thought occurred to me, especially because (from what I can tell right now) everything that needs sanitization is going to be calling GetParam for 'file'.

I have modified GetParam to call QuoteHTML before returning a value.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 5, 2015

Owner

To clarify, my comments about sanitizing are in regards to file names. There are quite a few calls where this isn't checked for validity.

Owner

ajgraves commented Sep 5, 2015

To clarify, my comments about sanitizing are in regards to file names. There are quite a few calls where this isn't checked for validity.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 6, 2015

Owner

@AlexDaniel to answer your question about what is breaking your attempt to write into an arbitrary path, the answer can be found in sub DoPosting. Any slashes in the file name are removed, and then all leading periods are removed.

So it seems like the arbitrary file path is a non-issue, however I'll keep checking the code to make sure. Work continues on the XSS portion as well.

Owner

ajgraves commented Sep 6, 2015

@AlexDaniel to answer your question about what is breaking your attempt to write into an arbitrary path, the answer can be found in sub DoPosting. Any slashes in the file name are removed, and then all leading periods are removed.

So it seems like the arbitrary file path is a non-issue, however I'll keep checking the code to make sure. Work continues on the XSS portion as well.

@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 6, 2015

@ajgraves are you sure? Because, even if it is removed, sub DoPostingDiscuss does this:

    AppendPage(GetParam('file'), GetParam('text'), GetParam('uname'),
      GetParam('url'));

That is, it GetParam-s the original untreated value. Given that I was unable to write into arbitrary path (although I did create some pages with dots and slashes), there is probably something that strips it, or at least affects it in some way. But yeah, I could also be wrong.

AlexDaniel commented Sep 6, 2015

@ajgraves are you sure? Because, even if it is removed, sub DoPostingDiscuss does this:

    AppendPage(GetParam('file'), GetParam('text'), GetParam('uname'),
      GetParam('url'));

That is, it GetParam-s the original untreated value. Given that I was unable to write into arbitrary path (although I did create some pages with dots and slashes), there is probably something that strips it, or at least affects it in some way. But yeah, I could also be wrong.

@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 6, 2015

Oh, and also, when I was trying to write into arbitrary path, there was no page file created. So it just failed to write it, it did not write into sanitized file path.

AlexDaniel commented Sep 6, 2015

Oh, and also, when I was trying to write into arbitrary path, there was no page file created. So it just failed to write it, it did not write into sanitized file path.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 6, 2015

Owner

You know what @AlexDaniel, I was thinking that after I typed my response. So in sub DoPosting I'm going to have to call SetParam on 'file' to replace it with the "sanitized" file name.

Thinking about it, the failure to write to an arbitrary path may be more related to system permissions then. I know on my server CGI scripts are executed under my user name, so it would be unable to write in any path where my user ID (or group) did not have write access.

Owner

ajgraves commented Sep 6, 2015

You know what @AlexDaniel, I was thinking that after I typed my response. So in sub DoPosting I'm going to have to call SetParam on 'file' to replace it with the "sanitized" file name.

Thinking about it, the failure to write to an arbitrary path may be more related to system permissions then. I know on my server CGI scripts are executed under my user name, so it would be unable to write in any path where my user ID (or group) did not have write access.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 7, 2015

Owner

I believe I have this corrected in commit a009539, however there's a few other areas I need to check. Since GetParam now calls QuoteHTML then there shouldn't be an issue going forward, however any existing "bad" data would be a problem.

Owner

ajgraves commented Sep 7, 2015

I believe I have this corrected in commit a009539, however there's a few other areas I need to check. Since GetParam now calls QuoteHTML then there shouldn't be an issue going forward, however any existing "bad" data would be a problem.

@ajgraves

This comment has been minimized.

Show comment
Hide comment
@ajgraves

ajgraves Sep 7, 2015

Owner

I also "stole" three things from Oddmuse's Refactoring page:

  1. Replace 'use vars' with 'our'
  2. Two-var open
  3. Bareword file handle
Owner

ajgraves commented Sep 7, 2015

I also "stole" three things from Oddmuse's Refactoring page:

  1. Replace 'use vars' with 'our'
  2. Two-var open
  3. Bareword file handle
@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 7, 2015

It's OK, that's why this page is public :)

However, I don't think that I will have a chance to review this commit in detail. It is a bit too hard for me when changes are not split into smaller commits. Some time later I will probably try to shove shitty data into various places (this is usually much easier than trying to find an error in the code). Not any time soon though.

AlexDaniel commented Sep 7, 2015

It's OK, that's why this page is public :)

However, I don't think that I will have a chance to review this commit in detail. It is a bit too hard for me when changes are not split into smaller commits. Some time later I will probably try to shove shitty data into various places (this is usually much easier than trying to find an error in the code). Not any time soon though.

@ajgraves ajgraves added this to the 0.50 milestone Sep 11, 2015

@AlexDaniel

This comment has been minimized.

Show comment
Hide comment
@AlexDaniel

AlexDaniel Sep 11, 2015

If you feel that it is fixed then you can close this issue. If I ever have time to find more stuff I'll leave a comment or write an email. Thanks!

AlexDaniel commented Sep 11, 2015

If you feel that it is fixed then you can close this issue. If I ever have time to find more stuff I'll leave a comment or write an email. Thanks!

@ajgraves ajgraves closed this Sep 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment