You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/05/04 15:00:31 UTC
[GitHub] [couchdb] jiahuili430 opened a new pull request #3541: Fix bug in replicator authentication for password contains @
jiahuili430 opened a new pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
When the username and password contains @, the replicator authentication will break.
This PR fixes that bug by unquoting the credentials.
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
## Testing recommendations
make eunit apps=couch_replicator suites=couch_replicator_auth_session
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
## Related Issues or Pull Requests
This fixes #2892
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [x] Code is written and works correctly
- [x] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva edited a comment on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832192773
> I am pretty sure it is exploitable.
I guess I still don't see how it's related to this replicator PR. All this logic happens on the replicator (client) side, any client could or would specify basic auth credential in the exact same way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] jiahuili430 commented on a change in pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
jiahuili430 commented on a change in pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#discussion_r626080471
##########
File path: src/couch_replicator/src/couch_replicator_auth_session.erl
##########
@@ -288,7 +288,9 @@ extract_creds_from_url(Url) ->
Prefix = lists:concat([Proto, "://", User, ":", Pass, "@"]),
Suffix = lists:sublist(Url, length(Prefix) + 1, length(Url) + 1),
NoCreds = lists:concat([Proto, "://", Suffix]),
- {ok, User, Pass, NoCreds}
+ User1 = mochiweb_util:unquote(User),
+ Pass1 = mochiweb_util:unquote(Pass),
Review comment:
Thanks for reviewing my PR @iilyak, I replaced the quoted line... and waiting for the tests to pass now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832191298
It looks like we are encoding the username before constructing the database name for peruserdb use case https://github.com/apache/couchdb/blob/3.x/src/couch_peruser/src/couch_peruser.erl#L318
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832188145
I am pretty sure it is exploitable. I didn't try the whole PR. However in remsh you can do this.
1. I created a file in my home directory `echo "hello" > ~/bar`
2. My couchdb instance is located in the following dir `~/dev/couchdb`
3. I entered erlang shell `ERL_LIBS=(pwd)/src erl`
4. Then I issued the following (see https://github.com/apache/couchdb/blob/3.x/src/couch/src/couch_file.erl#L469 and https://github.com/apache/couchdb/blob/3.x/src/couch/src/couch_file.erl#L413)
```
f(Fd), {ok, Fd} = file:open(mochiweb_util:unquote("..%2F..%2Fbar"), [read, raw, binary,append]).
file:write(Fd, <<"garbage">>).
```
5. Finally I verified the content of a `~/bar` file
```
✦ ❯ cat ~/bar
hello
garbage⏎
```
The atacker can corrupt any file on the filesystem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva edited a comment on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832192773
> I am pretty sure it is exploitable.
I guess I still don't see how it's related to this replicator PR. All this logic happens on the replicator (client) side, any client could or would specify basic auth credentials in the exact same way...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832167907
> @iilyak wouldn't the users be able to encode the username in basic auth headers anyway even outside the replicator code?
True however we never attempt to unquote the user. This means that it would stay encoded. Encoded value is ok to use as filename. When we are trying to unquote it we are opening can of worms.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832197325
@iilyak ah no worries, thanks for taking a look!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832194245
> > I am pretty sure it is exploitable.
>
> I guess I still don't see how it's related to this replicator PR. All this logic happens on the replicator (client) side, any client could or would specify basic auth credential in the exact same way?
I forgot it is about client side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832186985
@iilyak
We don't unquote usernames during authz because they are in a base64-encoded blob in the `"Authorization" : "Basic ..."` headers so there is no reason to quote them. When they are specified as the userinfo part of the URL some characters cannot be represented in that section of the URL, so they users would percent-encode (quote) them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva edited a comment on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832192773
> I am pretty sure it is exploitable.
I guess I still don't see how it's related to this replicator PR. All this logic happens on the replicator (client) side, any client could or would specify basic auth credentials in the exact same way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva edited a comment on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832186985
@iilyak
We don't unquote usernames during authz because they are in a base64-encoded blob in the `"Authorization" : "Basic ..."` headers so there is no reason to quote them. When they are specified as the userinfo part of the URL some characters cannot be represented in that section of the URL, so the users would percent-encode (quote) them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832029417
The build looks broken - the CI fail looks unrelated to the change. I'll open a PR to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832196457
Great work @jiahuili430 !!!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#discussion_r626023338
##########
File path: src/couch_replicator/src/couch_replicator_auth_session.erl
##########
@@ -288,7 +288,9 @@ extract_creds_from_url(Url) ->
Prefix = lists:concat([Proto, "://", User, ":", Pass, "@"]),
Suffix = lists:sublist(Url, length(Prefix) + 1, length(Url) + 1),
NoCreds = lists:concat([Proto, "://", Suffix]),
- {ok, User, Pass, NoCreds}
+ User1 = mochiweb_util:unquote(User),
+ Pass1 = mochiweb_util:unquote(Pass),
Review comment:
I think we should call [`chttpd:unquote/1`](https://github.com/apache/couchdb/blob/main/src/chttpd/src/chttpd.erl#L678) here instead. This would make moving away from mochiweb framework easier in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832096179
My concern is uri encoding of a user name. The `couch_peruser` uses user name as part of a database name. Which means that it might be possible to inject things into filename (3.x puts dbname in a filename). I don't have proof or exploit at the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832153299
@iilyak wouldn't the users be able to encode the username in basic auth headers anyway even outside the replicator code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak edited a comment on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak edited a comment on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832096179
My concern is uri encoding of a user name. The `couch_peruser` uses user name as part of a database name. Which means that it might be possible to inject things into filename (3.x puts dbname in a filename). I don't have proof or exploit at the moment.
Please do not merge yet. It needs more reviews/impact analysis.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#issuecomment-832192773
> I am pretty sure it is exploitable.
I guess I still don't see how it's related to this replicator PR. All this logic happens the replicator (client) side, any client could or would specify basic auth credential in the exact same way?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3541: Fix bug in replicator authentication for password contains @
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3541:
URL: https://github.com/apache/couchdb/pull/3541#discussion_r626055601
##########
File path: src/couch_replicator/src/couch_replicator_auth_session.erl
##########
@@ -288,7 +288,9 @@ extract_creds_from_url(Url) ->
Prefix = lists:concat([Proto, "://", User, ":", Pass, "@"]),
Suffix = lists:sublist(Url, length(Prefix) + 1, length(Url) + 1),
NoCreds = lists:concat([Proto, "://", Suffix]),
- {ok, User, Pass, NoCreds}
+ User1 = mochiweb_util:unquote(User),
+ Pass1 = mochiweb_util:unquote(Pass),
Review comment:
There are pros and cons to either way. For example `chttpd` is not mentioned as a dependency in the app file. However we do call functions from `chttpd`. I am fine with leaving it as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org