-
Notifications
You must be signed in to change notification settings - Fork 193
Suggestion: PG::Connection#complile #726
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
Open
intale
wants to merge
5
commits into
ged:master
Choose a base branch
from
intale:pg_connection_compile
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+259
−3
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6b746e9
Implement PG::Connection#complile
intale f5893b6
Add a generic method to PG::TypeMap to retrieve encoders
larskanis f94c52f
embed_params: Handle hash parameter format and OID properly
larskanis 9a055de
Add some documentation and small improvement to `query_param_encoders`
larskanis 807c591
embed_params: Fix escaping bytea on legacy non standard conforming st…
larskanis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IME, these kinds of not-quite-parsers are vulnerable to SQL injection.
Perhaps a stronger or more specific statement about how this should/not be used with user input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. If there is any mistake in regexp - the result may end up containing sql injection. For example
may result in
instead this
if we forget to cover regular string literal(e.g.
'my str') inPLACEHOLDER_RE. Thus, as long as we cover all possible variations of string literals and escape the value - we are safe. Fortunately I covered all possible scenarios in tests. As an additional measure - I can put here a guard that fails on new postgresql versions and a dev should check if any new string literals appear in a new postgresql version and adjust the implementation accordingly. Something like this(in#embed_params_and_check):What do you think, guys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe we should make a stronger statement about security. Putting SQL and user data together into one string is always more risky than keeping them separate.
On the other hand to exploit a incomplete or broken regex it's necessary to have a strange SQL placeholder string. But this string must not contain any unchecked user inputs anyway, so cannot be manipulated. And the
paramsitems go through theescapeorescape_byteamethods provided by libpq, so that they should be safe.So I think it's not necessary to put a guard for new PostgreSQL versions. But maybe someone speaking native English can improve my wording or make it more specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately
#escapedoes not fully protect us from sql injections. This is because we have to support every possible string literals here, but#escapeonly ensures there is correct amount of'in the string. Thus, if some regexp contains a mistake - an injection becomes possible for other string literals. In the example bellow you can see an injection using$$in case this variant is not handled properly(you can simply delete that variant to reproduce the issue):Thus, such issue might appear because we basically replace all appearances of positional variable in the string thanks to
gsuband incorrect/absent regexp. This is what I meant in my previous message when I was suggesting to add another guard against new potential string literals in future PostgreSQL versions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@intale I understand your concern. But what I mean is that the SQL string must be manipulated, in order to exploit a flaw in the regex, which should not be possible for an attacker. An attacker can change the
params, but he cannot exploit a flaw in the regex.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explanation. That is true. Let me rephrase my concern. My intention of adding a guard is to prevent next case:
%% my str %%But if we would have a guard that demands manual check of new PostreSQL version support before succeeding the build - we would be more safe than without it.