Closed heikojansen closed 1 year ago
OK, I have forked this repo and made some changes to the code: https://github.com/nginx-shib/nginx-http-shibboleth/compare/master...heikojansen:nginx-http-shibboleth:master
I did not come up with these changes myself but simply copied what I considered relevant from these two commits:
A very quick test after rebuilding nginx (1.23.1) and ngx_http_shibboleth seems to indicate that these changes indeed fix the crash I have reported here!
I currently don't understand why PCRE2 build flags were changed in that second commit and I also need to run a few more tests but at least it looks like the original assumption about the cause of the SIGSEGV was correct and we're headed in the right direction.
Any updates here?
I'm running nginx 1.23.1 with ngx_http_shibboleth built out of your fork and concur that it fixes the segfault issue. If there's no obvious issues or improvements to make, I think it makes sense to pull the fix into the main repo.
Thanks for the confirmation of the fix! I did not have the time to create a proper pull request, so there's been no progress on my side. If the maintainers of the project decide to integrate the changes by themselves that would also be fine by me. Otherwise, I hope I'll be able to create a pull request sometime next week.
Thank you heikojansen for sorting this out. To help the maintainers, I have submitted a PR pulling in your changes. Hopefully it is acceptable to all (this is a load-bearing component for some of our stuff, hence my interest in seeing this merged).
Well .... that explains why I just saw this error message when I tried to submit my pull request:
Pull request creation failed. Validation failed: A pull request already exists for heikojansen:nginx_compat_1-23-0.
I was just now preparing a new branch and adding proper commits with comments. Fortunately you at least submitted that pull request only after I added all intended changes.
OK, in the end it only matters that the issue is resolved, so ...
Oops! Sorry for stepping on your toes there. I could withdraw it and turn it over to you if you'd like (I had reviewed it and made sure it worked on our systems =) ).
Beautiful, thanks. I closed the one I opened, sorry for the noise.
Yeah, I actually decided to recreate the branch and submit a pull request of my own so there's now PR #50 to be reviewed and - hopefully - included...
Thanks @heikojansen for the PR - #50 is merged 👍
I recently tried to update our nginx to v1.23.1 (from 1.21.6). The latest version of the ngx_http_shibboleth module was successfully recompiled against the new nginx sources and the server can be started but crashes as soon as I try to access a protected path.
The nginx config is basically this:
Accessing
/auth/shib
usingcurl http://host/auth/shib
results in a core dump.I recompiled nginx with the debug option and debug symbols. Based on the log file I assume that the FCGI subrequest to the shibauthorizer has completed successfully and that the crash happens either when preparing the subrequest reponse to return from "/shibbolethauthorizer" or when analyzing this response in the context of the "/auth/shib" main request:
The SIGSEGV core dump stack is structurally the same for every test:
Cf. https://github.com/nginx/nginx/blob/master/src/http/modules/ngx_http_charset_filter_module.c#L330
So the "Location" header has apparently been parsed successfully but something's wrong with the charset info.
The nginx changelog mentions for v1.23.0:
And since some fiddling with header values takes place in ngx_http_shibboleth_module.c I came to the assumption that this might be the origin of the corrupted data.
Would be great if you could have a look at this. For now I'll downgrade back to nginx 1.21.6 where things worked fine before.
Heiko