Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
/((([\w -]+)|("[\w -]+"))( *, *)?)+/

http://refiddle.com/18ql

I'm trying to use a PHP regex to sanitize a user input for a list of fonts. The above one seems to work nicely, but also feels a bit long and redundant, having two [\w -]*s, and allowing a trailing comma, and possibly stuff I can't see. Can this be more efficient (smaller, less redundant, more secure)?

Examples

The regex should match these:

Roboto
"Roboto"
"Roboto Condensed"
Roboto Condensed
Roboto Condensed, Roboto
"Roboto Condensed", Roboto
Roboto Condensed, "Roboto"
"Roboto Condensed", "Roboto", sans-serif

or any string that matches the W3C specification for a CSS font-family property

It must be able to match these, but not any preceding (e.g. /*) or succeeding (e.g. ; DROP TABLE foo) string that might cause errors and open up exploits. So, it should match the font-family list (bold) in the following example, but not the surrounding potentially malicious characters.

/*"Roboto Slab", "Helvetica Neue", "Arial", sans-serif; DROP TABLE foo --

Similarly, I want it to also remove strings that someone might not know would produce broken code:

font-family:"Roboto Slab", "Helvetica Neue", "Arial", sans-serif; /* My fonts */

My input-handling function:

$userFontName = sanitizeFontName($_GET['font'])


function sanitizeFontName($fontName)
{
    $match = null;
    preg_match('/((([\w -]+)|("[\w -]+"))( *, *)?)+/', $fontName, $match); // Only get a W3C font list. Proven here: http://refiddle.com/18ql
    return $match[0]; // only one
}

Demo

share|improve this question
1  
Is it really necessary to filter out SQL injection attempts via regex? If the user is intentionally trying to break the system, they deserve to end up with invalid CSS. –  cimmanon Jul 20 at 20:08
    
@cimmanon yes, because I'm not as clever as a good hacker, who might be able to figure out code injection I don't know and use it to do things to the server. I'm not a security professional, I just know I have to sanitize user input to guarantee it's only what I want, and nothing else. –  Supuhstar Jul 20 at 20:16
1  
It shouldn't be possible for user input to do malicious things with text input unless you're running eval on it (which is bad and you shouldn't do) or you fail to insert it into the database correctly (in which case, Bobby Tables is here to help). The only other "bad" thing a user could really do is XSS injections, but those shouldn't be possible from CSS. –  cimmanon Jul 20 at 20:21
5  
The regexp is far from matching the W3C definition of allowed font names. It allows any Unicode characters, though some characters need to be escaped using CSS escape notation and some imply a need for quotation marks around the name. –  Jukka K. Korpela Jul 20 at 20:51

3 Answers 3

up vote 10 down vote accepted

Your approach is completely wrong.

There is a difference between escaping and validation

Validation is the act of making sure a piece of input conforms to certain rules (be it logical, or business rules). It should not be used for security. You can't catch all possible edge cases with a blacklist, don't try.

Escaping (or Sanitization) is the act of escaping characters with special meaning and replacing them with another character or a string that will have the meaning of the literal character in the target context (for example, replacing < with &lt; in HTML). Escaping can and should be used for making sure nothing with special meaning enters the target context (HTML, MySQL query, JavaScript, etc).


In your case, the context is a MySQL query, you're trying to prevent SQL injection. The solution to SQL injection is not validation. It's prepared statements. So:

$pdo = //Instantiate PDO connection.
$stmt = $pdo->prepare("INSERT INTO fonts(id) VALUES (:id)");
$stmt->bindValue(":id", "1; DROP TABLE fonts -- "); //Unsafe input
$stmt->execute(); //Will not drop the table. No results will be returned.

With an insert, the entire string 1; DROP TABLE fonts -- will be inserted.

Sometime in the future (when you try to fetch the font from the database), and before you output it into CSS, you'll want to escape the string for this new context. CSS escaping is something that hasn't a native PHP API, but there are probably libraries around.

share|improve this answer
    
I'd also like to address them putting in a wrong, but not malicious string, like in my example with font-family:"Roboto Slab", "Helvetica Neue", "Arial", sans-serif; /* My fonts */ –  Supuhstar Jul 21 at 15:41
1  
That should be done client-side, this is not the concern of the server. –  Madara Uchiha Jul 21 at 15:53
    
I agree with this answer. I find the term "sanitization" to be misleading. I always say either "escaping" (for correctness and security) or "validation" (for data quality). –  200_success Jul 22 at 3:04
    
Server-side validation may also be appropriate, depending on how the application is architected. –  200_success Jul 22 at 3:05
    
@200_success I like to differentiate between data integrity and business validation. Data integrity is done by the RDBM, while business validation is done by your app. –  Madara Uchiha Jul 22 at 7:38

I'll tell you, I definitely had trouble reproducing your expression. The Refiddle you linked to was using JavaScript's regex engine, not PHP's PCRE.

Now, what you're doing is something that is complicated. Just so everyone knows, here is the official documentation regarding fonts of the web.

If we dig a little, we will find a sentence saying:

Therefore, only the range of ISO 10646 characters will be used to qualify matches for the font face name.

Now, the definition of "ISO 10646" will give us something like:

The Universal Character Set (UCS), defined by the International Standard ISO/IEC 10646, contains nearly one hundred thousand abstract characters.

From the way I see, your check of \w doesn't cover this. The documentation you linked to specifically say:

Some font formats allow fonts to carry multiple localizations of the family name.

And the example directly below that covers the locale of (I'm pretty sure) Japan. The \w word check often relies on the user's localization configuration, which is in no way subject to a single standard.


I don't know how you're using this information, but if it's to just style a page, let the user enter what they want. The page will break harmlessly and it will be their loss. Styling with a /* or DROP TABLE will only make their webpage less stylish! If you're storing this information, same principle goes, just make sure to used prepared statements when inserting into your database.

share|improve this answer
    
I'm using this information to allow a CSS framework to be customized. Their input will be put into an interpreter and processed, possibly stored into a database in the future. I need to give them the freedom to provide legitimate information, to protect myself from malicious text, and to protect them from malformed strings if it is beyond their skill level –  Supuhstar Jul 21 at 0:48
1  
I quoted that documentation because it gives an exact list of characters that are usable in the property. You must find out how to implement this wide character set in your regular expression. –  Alex L Jul 21 at 1:15

I would recommend dumping the idea of free form text. While I don't understand your use-case completely, I suggest giving the user a list to choose from. Allow the user to pick or sort values from your list. On the back end, you would only allow values from the sanctioned list and this will ease the coding quite a bit.

share|improve this answer
    
I intend to allow the user to provide any font they might have on their site, so a dropdown won't work. Just like @AlexL's answer, I give this one -1 for not being an improvement of my code, or telling me how to improve it, or that it cannot be improved, instead opting to tell me how to conceptually build a project I have not described. –  Supuhstar Jul 21 at 0:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.