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

Certificate fields injection via %D in ERR_SECURE_CONNECT_FAIL #306

Conversation

chtsanti
Copy link
Contributor

@chtsanti chtsanti commented Oct 17, 2018

%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped
when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL
template. This bug affects all ERR_SECURE_CONNECT_FAIL page templates
containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D
with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other
non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.

… certificate

The error page allows injecting snippets using the distinguished name of
untrusted certificates via the %D template parameter.
This patch quote information retrieved from  remote certificates using the
%ssl_subject, %ssl_ca_name and %ssl_cn error details formating codes.

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

This is a Measurement Factory project
@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Oct 17, 2018
Copy link
Contributor

@yadij yadij left a comment

Choose a reason for hiding this comment

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

LGTM as a workaround

@rousskov rousskov added the S-waiting-for-committer privileged action is expected (and usually required) label Oct 20, 2018
@rousskov
Copy link
Contributor

@chtsanti, if you have not asked Nikolas for permission to publish his name and company affiliation, please do so. Besides that, this PR is on your side -- as you probably know, it needs PR description fixes and an M-cleared-for-merge label.

@rousskov
Copy link
Contributor

rousskov commented Oct 20, 2018

SSL Interception

Is this bug specific to intercepting Squids? Squid doing SslBump (a.k.a. SSL inspection)? Any Squid?

Injection into default ssl error page

Let's be explicit: ERR_SECURE_CONNECT_FAIL.

However, this injection is not limited to the default ERR_SECURE_CONNECT_FAIL page. It could happen whenever the admin uses %D, right?

@yadij
Copy link
Contributor

yadij commented Oct 22, 2018

However, this injection is not limited to the default ERR_SECURE_CONNECT_FAIL page. It could happen whenever the admin uses %D, right?

Any error page which uses %D and there is a TLS error to be displayed which embeds a value from certificate field(s). Otherwise %D does not expand to anything useful.

@chtsanti
Copy link
Contributor Author

@chtsanti, if you have not asked Nikolas for permission to publish his name and company affiliation, please do so.

I remove his name for now, I am waiting his response.

Besides that, this PR is on your side -- as you probably know, it needs PR description fixes and an M-cleared-for-merge label.

OK.

However, this injection is not limited to the default ERR_SECURE_CONNECT_FAIL page. It could happen whenever the admin uses %D, right?

Any error page which uses %D and there is a TLS error to be displayed which embeds a value from certificate field(s). Otherwise %D does not expand to anything useful.

Only in ERR_SECURE_CONNECT_FAIL, only for this error we are completing the ErrorState::detail member which stores the error details.

@rousskov rousskov changed the title SSL Interception: Injection into default ssl error page via untrusted… Certificate fields injection via %D in ERR_SECURE_CONNECT_FAIL Oct 22, 2018
@rousskov
Copy link
Contributor

@chtsanti, I have adjusted the PR title and description based on your last remarks. Before clearing this PR for merge, please check that the updated title/description match reality.

@chtsanti chtsanti removed M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels S-waiting-for-committer privileged action is expected (and usually required) labels Oct 23, 2018
@yadij yadij added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label Oct 23, 2018
@yadij
Copy link
Contributor

yadij commented Oct 23, 2018

Reformatted the description to fix line lengths making the M-failed-description removal valid.

@rousskov
Copy link
Contributor

@chtsanti, FYI: The M-failed-description label is managed by Anubis, not you. After the formatting was fixed, Anubis should have removed stale M-failed-description when revisiting this PR (it does not happen in real-time yet, unfortunately).

The only purple M- label that humans manage is M-cleared-for-merge (Amos set that for you in this PR AFAICT).

@rousskov
Copy link
Contributor

@yadij, FYI: The line length limit is 72, not 65. The reason we keep titles at or below 65 characters is because the PR number gets auto-added to the title during merge commit formation (by GitHub or Anubis). See https://github.com/measurement-factory/anubis#commit-message

squid-anubis pushed a commit that referenced this pull request Oct 23, 2018
%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped
when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL
template. This bug affects all ERR_SECURE_CONNECT_FAIL page templates
containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D
with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other
non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.
@squid-anubis squid-anubis added M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Oct 23, 2018
@chtsanti chtsanti deleted the Injection_into_default_ssl_error_page branch October 24, 2018 09:00
squidadm pushed a commit to squidadm/squid that referenced this pull request Oct 25, 2018
…-cache#306)

%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped
when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL
template. This bug affects all ERR_SECURE_CONNECT_FAIL page templates
containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D
with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other
non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.
yadij pushed a commit that referenced this pull request Oct 26, 2018
%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped
when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL
template. This bug affects all ERR_SECURE_CONNECT_FAIL page templates
containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D
with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other
non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.
squidadm pushed a commit to squidadm/squid that referenced this pull request Oct 27, 2018
…-cache#306)

%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL template. This bug affects all
ERR_SECURE_CONNECT_FAIL page templates containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.
yadij pushed a commit that referenced this pull request Oct 27, 2018
%ssl_subject, %ssl_ca_name, and %ssl_cn values were not properly escaped when %D code was expanded in HTML context of the ERR_SECURE_CONNECT_FAIL template. This bug affects all
ERR_SECURE_CONNECT_FAIL page templates containing %D, including the default template.

Other error pages are not vulnerable because Squid does not populate %D with certificate details in other contexts (yet).

Thanks to Nikolas Lohmann [eBlocker] for identifying the problem.

TODO: If those certificate details become needed for ACL checks or other non-HTML purposes, make their HTML-escaping conditional.

This is a Measurement Factory project.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
4 participants