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

Encode url parameters #1201

Merged
merged 4 commits into from Jun 30, 2018

Conversation

Projects
None yet
4 participants
@bazzargh
Copy link
Contributor

bazzargh commented May 23, 2018

BaseDataSource did not properly encode url parameters, meaning that users could
not log in if their password contained illegal characters. The error can be
reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal
characters for query parameters).

DriverTest then failed because it did not encode the parameters either.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 23, 2018

Codecov Report

Merging #1201 into master will increase coverage by 0.37%.
The diff coverage is 68.75%.

@@             Coverage Diff              @@
##             master    #1201      +/-   ##
============================================
+ Coverage     68.67%   69.05%   +0.37%     
- Complexity     3825     3946     +121     
============================================
  Files           172      173       +1     
  Lines         15991    16528     +537     
  Branches       2603     2773     +170     
============================================
+ Hits          10982    11413     +431     
- Misses         3776     3843      +67     
- Partials       1233     1272      +39

@bazzargh bazzargh force-pushed the bazzargh:urlencode branch from bbae092 to 6e6caf3 May 23, 2018

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented May 24, 2018

@bazzargh so it fails on Java 6 due to the fact that it java 6 does not have constructor AssertionError(java.lang.String,java.io.UnsupportedEncodingException)

Also I don't actually see a test for the user password mentioned above ?

@bazzargh

This comment has been minimized.

Copy link
Contributor Author

bazzargh commented May 24, 2018

I didn't add the test because the existing tests don't have any tests like that at all - they rely on the passwords set externally for the test and postgres users. However, I tried to add one and discovered this whole thing relies on a second oddity. The password only appears in the URL when the datasource is part of a JNDI test. BaseDataSource.getReference creates reference properties for username/password as well as other properties; when unpacking them again in setFromReference, it sets the 'special' properties correctly but then adds all reference properties back into the ds's properties object, including those that had already been handled specially.

I think I should patch both issues, the encoding issue is still a problem if any other properties include special characters, like APPLICATION_NAME. But the 'special' set of properties shouldn't leak back into the standard properties too, so I'll fix that.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented May 24, 2018

@bazzargh
Yes the data source code handling of properties is in need of some attention.
Is it even possible to use these special characters in APPLICATION_NAME ?

@bazzargh bazzargh force-pushed the bazzargh:urlencode branch 2 times, most recently from ba75a53 to 877a13c May 24, 2018

@bazzargh

This comment has been minimized.

Copy link
Contributor Author

bazzargh commented May 24, 2018

@davecramer yes, you can set that with special characters - and also the database name, which can be anything you like. I've fixed up a couple of tests to exercise this

@vlsi

This comment has been minimized.

Copy link
Member

vlsi commented May 25, 2018

I think proper encoding/decoding makes sense.

I think we must test that otherwise we might end up in case the code fails to work for both encoded and plain passwords.

I think it might be just enough if we use special symbols for Travis PostgreSQL password. @bazzargh , what do you think?

@bazzargh

This comment has been minimized.

Copy link
Contributor Author

bazzargh commented May 25, 2018

@vlsi I think it would be better to set the application name to include special characters - particularly something like a % with non-hexadecimal after it, as that trips up URLDecoder if it's not been encoded.
The reason for using application name is that after these patches the password is no longer included in the urls generated by the datasource but other properties like application name are. But, as a double check - the tests need two passwords, one for the 'postgres' user and one for 'test', it wouldn't hurt to make one of those use special characters and leave one plain.
It may be unnecessary though, since the tests do actually cover this now. The 'bad' url generated by the datasource was eventually parsed by Driver.parseURL, where it was split at the wrong places and so the wrong password got sent to the backend. I added an explicit test that what BaseDataSource produces can be consumed by Driver in PGPropertyTest.java.

@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented May 25, 2018

@bazzargh +1 on APPLICATION_NAME
I still want to see special chars in the password, just in case pg doesn't allow them

@bazzargh

This comment has been minimized.

Copy link
Contributor Author

bazzargh commented May 25, 2018

@davecramer the reason this came up is because our DBA's create accounts with strong passwords, and we discovered that while we could use the password from the command line client, our java apps couldn't connect. The workaround was to generate a new password, but I dug in to why it had happened. During testing I was using ';/?:@&=+$,' as my password, as described above, so I'm anecdataly confirming this works in pg but not jdbc, you may want to set your tests up that way as another datapoint.

I only tested with url-special characters; I've no idea if there's any further restrictions on the password character set, eg do emoji work? But that's beyond the scope of this bug.

bazzargh added some commits May 23, 2018

fix: Encode url query parameters
BaseDataSource did not properly encode url parameters, meaning that users could
not log in if their password contained illegal characters. The bug can be
reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal
characters for query parameters). Encode the parameters. Strictly speaking the
parameter names should also be encoded but in this case they are a fixed set of
words which only consist of safe characters.

With the problem password, DriverTest also fails because it did not encode the
parameters either. Encode the parameters in the test too.
fix: do not leak password to URL
round-tripping a datasource through JNDI added the user and password
to the ds properties as well as to the instance variables - which was
not possible to do via setProperty. This may be a security issue if
the URL is logged, and was in part why passwords with non-url-safe
characters failed to connect in some circumstances.

Set properties using setProperty, so that consistent logic applies
to processing the user/password keys.

@vlsi vlsi force-pushed the bazzargh:urlencode branch from 877a13c to 873c52a Jun 30, 2018

@vlsi vlsi added this to the 42.2.3 milestone Jun 30, 2018

@vlsi vlsi merged commit 9f3838f into pgjdbc:master Jun 30, 2018

2 checks passed

codecov/project 69.05% (+0.37%) compared to 7018920
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: encode url query parameters DataSource (pgjdbc#1201)
BaseDataSource did not properly encode url parameters, meaning that users could
not log in if their password contained illegal characters. The bug can be
reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal
characters for query parameters). Encode the parameters. Strictly speaking the
parameter names should also be encoded but in this case they are a fixed set of
words which only consist of safe characters.

With the problem password, DriverTest also fails because it did not encode the
parameters either. Encode the parameters in the test too.

* fix: do not leak password to URL

round-tripping a datasource through JNDI added the user and password
to the ds properties as well as to the instance variables - which was
not possible to do via setProperty. This may be a security issue if
the URL is logged, and was in part why passwords with non-url-safe
characters failed to connect in some circumstances.

Set properties using setProperty, so that consistent logic applies
to processing the user/password keys.

rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018

fix: encode url query parameters DataSource (pgjdbc#1201)
BaseDataSource did not properly encode url parameters, meaning that users could
not log in if their password contained illegal characters. The bug can be
reproduced by setting the test user password to ';/?:@&=+$,' (a bunch of illegal
characters for query parameters). Encode the parameters. Strictly speaking the
parameter names should also be encoded but in this case they are a fixed set of
words which only consist of safe characters.

With the problem password, DriverTest also fails because it did not encode the
parameters either. Encode the parameters in the test too.

* fix: do not leak password to URL

round-tripping a datasource through JNDI added the user and password
to the ds properties as well as to the instance variables - which was
not possible to do via setProperty. This may be a security issue if
the URL is logged, and was in part why passwords with non-url-safe
characters failed to connect in some circumstances.

Set properties using setProperty, so that consistent logic applies
to processing the user/password keys.

@bazzargh bazzargh deleted the bazzargh:urlencode branch Sep 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.