Skip to content
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

fix: issue 1928 #1932

Closed
wants to merge 1 commit into from
Closed

fix: issue 1928 #1932

wants to merge 1 commit into from

Conversation

nickpalmer
Copy link

@nickpalmer nickpalmer commented Mar 6, 2024

This PR fixes the issues raised in #1928 and should prevent any other attacks using whitespace tricks as raised in the vulnerability report which initial prompted the breaking parenthesis addition.

It uses postgresql escape strings to prevent whitespace embedded in a value from being passed through to the SQL parser.

See: https://www.postgresql.org/docs/9.0/sql-syntax-lexical.html for information on this extension.

@nickpalmer nickpalmer changed the title fix: issue 1928 fix: issue #1928 Mar 6, 2024
@nickpalmer nickpalmer changed the title fix: issue #1928 fix: issue 1928 Mar 6, 2024
@jackc
Copy link
Owner

jackc commented Mar 7, 2024

The reason I went for parentheses around all values was to avoid any possibility of missing a dangerous value. It works properly for true query parameters. But as reported in #1928, it can break where the parameter is not actually a query parameter as PostgreSQL understands it.

With non-actual query parameters in mind, I don't think that parentheses are the right approach, even if only done for negative values. It is entirely possible for a SET statement as described in #1928 to be negative.

I think the actual approach is even simpler. Instead of wrapping with parentheses, wrap with spaces. That would prevent the accidental construction of a --. It would instead be a harmless - -. This would also prevent the accidental construction of a '' (two single quotes) which could cause two literal strings to be parsed as a literal string with a single quote in the middle. (Space was also mentioned in the original vulnerability report as a potential solution - I went with parentheses to in preference to having an invisible character be meaningful - but in light of #1928 I think it is worth it)

As far as the escape strings go, I don't think that is actually necessary. A PostgreSQL literal string is allowed to have newlines or any other character aside from \0 (NUL character). The only character that needs to be escaped is the ' (single quote).

@nickpalmer
Copy link
Author

The space solution is an interesting one.

I went to escape strings because that seemed like it would prevent other attacks we haven't thought of involving new lines, and because it makes the log of those queries much cleaner since they then don't log with newlines.

The other solution I considered was using the join of strings to substitute newlines with a pasted escaped newline.

str = strings.ReplaceAll(str, "\n", "'E'\\n''")


// Convert problematic line breaks to escape form
// See: https://www.postgresql.org/docs/9.0/sql-syntax-lexical.html Table 4.1
if strings.ContainsAny(str, "\b\f\n\r\t") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the scenario where the value has both a line break AND a - prefix, we get an invalid E('value') instead of (E'value')

@jackc
Copy link
Owner

jackc commented Mar 9, 2024

I ended up going for the space approach.

@jackc jackc closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants