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
Add Host HTTP header to MS-KKDCP requests #507
Conversation
In resolve_server(), portbuf is a local array variable. You appear to be storing an alias to this array in the resulting struct conn_state, which is then used after resolve_server() exits. I believe that's a memory error. Can you confirm that with valgrind and fix it? |
395e494
to
3144eed
Compare
Ah, you are right. I have addressed the issue in my latest patch. portbuf is now 6 chars wide, enough to store the decimal representation of UINT16_MAX. The local portbuf buffer is strncpy()ed to conn_state.http.port[6] char array. The overflow check after snprintf() ensures that neither portbuf nor http.port can overflow. |
@@ -700,6 +703,8 @@ add_connection(struct conn_state **conns, k5_transport transport, | |||
state->service_read = service_https_read; | |||
state->http.uri_path = uri_path; | |||
state->http.servername = hostname; | |||
strncpy(state->http.port, port, sizeof(state->http.port)-1); |
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.
If port is NULL this will crash.
I tracked
|
Modules can't add HTTPS entries, so that should be okay. However, please use strlcpy(), not strncpy(). |
I would add an assertion, not a check-and-return-EINVAL, unless you think there's a way to produce that condition using configuration. Adding a check also seems out of scope for this PR. |
Some webservers require a Host HTTP header for TLS connections with SNI (server name indicator). For example Apache HTTPD with a recent version of mod_nss aborts HTTPS requests without Host header with response '400 Bad Request' and error message: Hostname example.org provided via SNI, but no hostname provided in HTTP request The HTTP Host header is also required for virtual hosts. TLS SNI support was added in 4b6045a. https://bugzilla.redhat.com/show_bug.cgi?id=1364993 Signed-off-by: Christian Heimes <cheimes@redhat.com>
3144eed
to
6a4dd53
Compare
Thanks for your feedback. I have added an assert() and replaced strncpy() with strlcpy(). I didn't know about strlcpy() before. This function from FreeBSD is much nicer to use than strncpy(). |
Not sure why Travis is upset; this passes for me locally. |
I restarted the build. There was a known outage with the Travis infrastructure. |
For reference here is what happens with KRB5 >= 1.14 on the client:
Apache HTTP error log:
With a patched build of krb5-libs and krb5-workstation (http://koji.fedoraproject.org/koji/taskinfo?taskID=15189061), MS-KKDCP works again.
|
Pushed to master as 69c8662. |
@greghudson please push the fix to 1.14, too. |
I marked it for backport when I pushed to commit. Tom will be doing the cherry-pick onto the 1.14 branch when he next does release engineering work. |
@greghudson thank you! @frozencemetery explained to me how you are dealing with backports. |
Some webservers require a Host HTTP header for TLS connections with
SNI (server name indicator). For example Apache HTTPD with a recent
version of mod_nss aborts HTTPS requests without Host header with
response '400 Bad Request' and error message:
The HTTP Host header is also required for virtual hosts. TLS SNI support
was added in 4b6045a.
https://bugzilla.redhat.com/show_bug.cgi?id=1364993
Signed-off-by: Christian Heimes cheimes@redhat.com