Security issue with search functionality

[grandorf]

Received this from our internal security team after talking DNA with them:

As discussed, please see the PoC for sanitizing the user input. The below code uses regex to remove any characters that are not letters or numbers. I know this is not considered to be the nicest way of handling strings, but it showcases the idea well enough.

The reason for using the whitelisting solution is that any blacklisting solution might leave out some special characters. Even though the UrlEncode-like functions are widely used, in this case I didn’t check the underlying library to be 100% sure, it handles any user input properly.

The usual attack vectors to be tried against a library are 1. Special characters (XSS, SQL injection, Object injection, etc.) and 2. Too long strings (eg. Buffer overflows). As the below code allows maximum 20 characters long alphanumeric string, it should be safe to use. It can of course be longer as well, 20 is just an arbitrary number here, as I didn’t see anyone searching for 20+ characters with non-malicious intent realistically.

The client-side sanitization works in this case, as the client has to explicitly turn off part of the code, in order to bypass this. While this could be done by an attacker, this could only result in a self-hack (attacker can’t turn off victim’s code partially).

I hope I wrote the code bugless and everything is clear, but should any of this not be true, feel free to contact me anytime.

Many thanks and best regards, Norbert

https://github.com/youseedk/dna/blob/master/fractal-theme/assets/js/src/search-results.js line #32: const searchString = getSearchParams.get('q');

-->

function allow_chars_and_nums_only(var str) {
                str = str.replace(/[^\w\s]/gi, '');
                return str.slice(0,20);
}

const searchString = allow_chars_and_nums_only(getSearchParams.get('q'));

[havgry]

Can you (or someone else) explain to me how this poses a security risk, because I don't get it :)

• The input is only being used for a simple client-side lookup.
• Secondly, sanitizing the input client-side is pretty useless in an attempt to prevent a malicious attack.

I would categorize this threat as being theoretical at best.

[grandorf]

Lucas think we should do It and has volunteered ti implement this. I have opened the issue again and assigning him.