search

vichan-devel / vichan

Vichan is the most popular and widely used imageboard software in the world. It is a free, light-weight, fast, highly configurable and user-friendly imageboard software package.
https://vichan.info
Other
627 stars 196 forks source link

[code included] Fix injection when editing news pages, improve mod_edit_form code #448

Closed discomrade closed 2 years ago

discomrade commented 3 years ago

I would make this a pull request, but a sanity check first would be nice.

Relevant Lainchan issue: https://github.com/lainchan/lainchan/issues/94 "Edit post form does not escape HTML entities."

While their fix is sane, it ends up double-encoding special HTML characters when editing posts due to these two lines: https://github.com/vichan-devel/vichan/blob/master/inc/mod/pages.php#L1630-L1631

I believe the Lainchan fix is better than the lines above as it fixes the shared edit form itself instead of the individual page.

ctrlcctrlv commented 2 years ago

The real question here is who on Earth specified ENT_NOQUOTES for utf8tohtml (Cf. https://www.php.net/manual/en/function.htmlspecialchars.php)?

https://github.com/vichan-devel/vichan/blob/9745e9d8545ec38b3f83f38df742f33b6bd9e34a/inc/functions.php#L2282

Git blame says I added the text in 2013:

https://github.com/vichan-devel/vichan/commit/53f710060d1437f89291f26b10397a9c6c8bf4d2

But you can see ENT_NOQUOTES was already there.

That puts the blame for the constant on @savetheinternet: https://github.com/vichan-devel/vichan/commit/c9423a2c34a5b93bf9cf2c506acca713781e60b6

I have no idea how one would verify that a modification of utf8tohtml such that ENT_NOQUOTES is changed for ENT_QUOTES is safe. I encourage you to try to change it on your running Vichan instance…report back here how well that goes…if fine I'll patch it.

Other than that I'm not inclined to accept either patch as they ignore the root cause of the issue: shitty utf8tohtml function written by incompetent but lovable Australian cokehead.

discomrade commented 2 years ago

Other than that I'm not inclined to accept either patch

This is the right decision, I had forgotten about this issue report and it was poorly investigated on my end. In fact, even the title is misleading. (Essentially a hacky issue fix on lainchan's fork had confused me, then additionally merged vichans fix that had already fixed it in a more well-informed manner, but I didn't understand the rationale behind vichan's fix and thought the other was better.)

As for ENT_NOQUOTES, this appears to be the commit that introduces it: https://github.com/vichan-devel/vichan/commit/4a03c4c3cd6216405db69e3582df9ebf8809f057#diff-f3a2689b39e6a1a9532d63ed0bc25e99cafc51ae5d7486c07b1e68e5a9681a3f which is a fix (" XSS/bug with last commit to utf8tohtml(). ") for this commit: https://github.com/vichan-devel/vichan/commit/79d7bf54fa5fded7e10663e905fc99ceeadefb40#diff-f3a2689b39e6a1a9532d63ed0bc25e99cafc51ae5d7486c07b1e68e5a9681a3f which had previously used ENT_QUOTES in its manual solution. I don't see a rationale given for the change.

I'll start testing the ENT_QUOTES version now. Correct me if I'm wrong, but I think the only symptom would be double-encodes.

ctrlcctrlv commented 2 years ago

Hey @discomrade, thanks for your further research. I don't think there should be any double encodes with ENT_QUOTES—it should be safer vis-à-vis XSS as all it means is “replace all " with "”. If something takes the input of utf8tohtml and tries to do yet another encode on it then you'll get a double encode, but this shouldn't happen.

ctrlcctrlv commented 2 years ago

So?

ctrlcctrlv commented 2 years ago

I'm merging a solution as the most recent security bulletin means this is likely an issue, or will be in future.