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

certbot http -> https rewrite rules messes up GET params #3243

Closed
jasonmoofang opened this Issue Jul 6, 2016 · 7 comments

Comments

Projects
None yet
6 participants
@jasonmoofang
Copy link

jasonmoofang commented Jul 6, 2016

Hello,

An issue I encountered today: the rewrite rules added to apache config by certbot-auto to coerce http requests to https doesn't seem to be bulletproof.

In particular, GET params that are url-encoded, eg ?email=testest%2Byhbugfixer%40gmail.com, seem to end up getting encoded twice so that my application receives 'testest%2Byhbugfixer%40gmail.com' for the param value instead of the intended 'testest+yhbugfixer@gmail.com', if I make the initial request in http://

Notably, this does not happen if in place of the rewrite rules added by certbot-auto, I simply create a new virtual host like so:

<VirtualHost *:80>
ServerName mydomain.com
Redirect / https://mydomain.com/
</VirtualHost>

@pde

This comment has been minimized.

Copy link
Member

pde commented Jul 6, 2016

@sagi, thoughts?

@sagi

This comment has been minimized.

Copy link
Member

sagi commented Jul 6, 2016

@jasonmoofang, can you please share with us your configuration files, before and after certbot messes with them ?

@jasonmoofang

This comment has been minimized.

Copy link
Author

jasonmoofang commented Jul 7, 2016

hey @sagi ,

Unfortunately I no longer have the "before" configuration files, but I'm pretty sure this is the snippet inserted by certbot to coerce https:

RewriteEngine on
RewriteCond %{SERVER_NAME} =mydomain.com
RewriteRule ^ https://%{SERVER_NAME}%{REQUEST_URI} [END,QSA,R=permanent]

This is the contents of the virtual host file for my domain, if relevant:

<VirtualHost *:443>
ServerName mydomain.com
# !!! Be sure to point DocumentRoot to 'public'!
DocumentRoot /path/to/rails/public
RailsEnv production
<Directory /path/to/rails/public>
Require all granted
# This relaxes Apache security settings.
AllowOverride all
# MultiViews must be turned off.
Options -MultiViews
</Directory>
SSLEngine on
SSLCertificateFile /etc/letsencrypt/live/mydomain.com/cert.pem
SSLCertificateKeyFile /etc/letsencrypt/live/mydomain.com/privkey.pem
SSLCertificateChainFile /etc/letsencrypt/live/mydomain.com/chain.pem
<VirtualHost>

@jt2k

This comment has been minimized.

Copy link

jt2k commented Oct 12, 2016

I ran into this problem today. I think the fix is to add the NE (noescape) flag to the rewrite rules found here, but I'm not sure if that would create any undesired side effects.

@sagi

This comment has been minimized.

Copy link
Member

sagi commented Oct 13, 2016

Back then, during my research on this I bumped on this thread.

It seems that NE introduces undesired side effects, which were not entirely fixed in Apache 2.4.10 (although Apache claims otherwise).

I've been meaning to test it out; but didn't get to it.

Also, I believe that a change in escaping / encoding of the query string may harm current certbot users who already got themselves used to its behavior.

@diablodale

This comment has been minimized.

Copy link

diablodale commented Feb 9, 2017

Double-encoding a query is a well known issue with a resolution to use NE. There are cases where NE isn't the answer. However, for a blind wildcard redirect to move a client from http -> https it is important to not alter any part of the URL beyond the protocol (i.e. path, port, query, etc.).

Failure example:
http://foo.bar.com/apple?email=me%40domain.com
incorrectly redirecting to:
https://foo.bar.com/apple?email=me%2540domain.com

There is no reliable manner for the receiving code to know if the received query string is single url encoded or erroneously double url encoded. Therefore, there is no legit legacy workaround coded by current certbot users. Anything written by someone out there...isn't reliable.

Therefore, the resolution is to fix the core issue which is the use of QSA. It should instead be NE. This will result in...

Good example:
http://foo.bar.com/apple?email=me%40domain.com
correctly redirecting to:
https://foo.bar.com/apple?email=me%40domain.com

@sagi

This comment has been minimized.

Copy link
Member

sagi commented Feb 10, 2017

@diablodale thanks for your insights. I agree with your reasoning.

@bmw bmw closed this in #4204 Mar 3, 2017

@bmw bmw added this to the 0.13.0 milestone Mar 3, 2017

@bmw bmw modified the milestones: 0.12.1, 0.13.0 Mar 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment