Issue: Vanilla character encoding

mtschirs

All of the following can be found in the vanilla core:

htmlspecialchars(..., C('Garden.Charset', 'UTF-8'))
htmlspecialchars(..., 'ISO-8859-1'); // Smarty default
htmlspecialchars(..., 'UTF-8')
htmlspecialchars(...) // Defaults to 'ISO-8859-1' in PHP <= 5.3

From the PHP manual:

For the purposes of this function, the encodings ISO-8859-1 [and] UTF-8 [are] effectively equivalent, provided the string itself is valid for the encoding.

But what if C('Garden.Charset') is set to something more exotic? That is how you end up on http://seclists.org/ ...

Why even bother with C('Garden.Charset') then? Better make Vanilla UTF-8 only?

Bleistivt

@mtschirs said:
Why even bother with C('Garden.Charset') then? Better make Vanilla UTF-8 only?

Vanilla is pretty much UTF-8 only. At least I wouldn't try using a different encoding.

But really, this is the kind of issue to post on github.

for reference, see this discussion:
https://github.com/vanilla/vanilla/pull/2507

But what if C('Garden.Charset') is set to something more exotic? That is how you end up on http://seclists.org/ ...

Do you have an example for a working exploit?
Also, it is not a user-facing configuration setting. If someone creates a vunerability by changing this without a reason it is their fault.

peregrine

I think this is an example why posting on forum in addition to github, is optimal. since it is easier to find here than on github. and it is read by a larger population of readers than github..

@mtschirs very insightful discussions.

mtschirs

@Bleistivt schrieb:
Do you have an example for a working exploit?

The thing is, writing secure code should be a high, if not the highest priority for a PHP web application. Lets say someone in China installs Vanilla and sets the character encoding to some more exotic variant, but htmlspecialchars looks for dangerous characters according to ISO-8859-1. That cannot work well and surely will be a user facing issue once XSS gets injected into the forum...

@Bleistivt schrieb:
Vanilla is pretty much UTF-8 only.

And 'pretty much' secure, too. Let's remove the 'pretty much' here, ok?

@Bleistivt schrieb:
But really, this is the kind of issue to post on github.

Posting an issue on github means it might never get fixed though... This issue does not have a single fix, but requires a decision made by the community / the official maintainers.

mtschirs

@Bleistivt: I just read the discussion + your pull request you referenced on github . Can you explain to me what the vanilla stage branch is for exactly? Shouldn't this be fixed in master?

Bleistivt

See http://vanillaforums.org/discussion/27971/pull-requests-you

Basically stage is where larger pull requests are tested before they are merged into master.

...At least it was used like that. I have been told that the stage branch is not used so much anymore.

peregrine

another encoding related issue filed a 1 1/2 years ago.

https://github.com/vanilla/vanilla/issues/1773

not sure how it relates to current vanilla version

mtschirs

From the current 2.2b1 announcement thread:

@Linc schrieb:
Here's some of what's new in 2.2:
...

• Harmonize charset in htmlspecialchars.
  ...

Did I miss something?

R_J

@mtschirs said:
Did I miss something?

https://github.com/vanilla/vanilla/pull/2507

mtschirs

@R_J: Yes, this was a good start, but why did they only convert a select few of all existing htmlspecialchars in the codebase? Now we have 3 competing standards instead of 2... isn't that the opposite of "harmonize"?

peregrine

edited.

Linc

The conversations on that PR gives a little background, I think. The best way to approach this would be to create a htmlEsc() function that wraps our repetitious implementation of htmlspecialchars() in a canonical way, and use that everywhere it's needed in Vanilla.

R_J

Well, in this case "they" have been "me" and I either
a) simply haven't seen the other files
b) was only looking for a special pattern
c) thought changing the other files is too much of a core change
d) left the other work for some other people
e) "I've harmonized all that uses any charset parameter"
f) am only a noob who is already happy when I change only a selected few files and would have never commited any Pull Request if it had to be perfect.

Honestly I do not remember.

mtschirs

@R_J I didn't want to come off as negative, I simply didn't make the connection between what Linc wrote about 2.2 <-> your pull request. It is a very good start indeed.

@Linc: I am not convinced of "htmlEsc" yet, would need an "htmlUnesc" too. Also, "htmlspecialchars" is easy to spot and every PHP dev knows what it does. It would improve readability if we just stick with it and appropriately overwrite default params. This will be a non-issue in PHP >= 5.6 anyway due to the default_charset setting.

mtschirs

See https://github.com/vanilla/vanilla/issues/3047 - make Vanilla UTF-8 only