-
Notifications
You must be signed in to change notification settings - Fork 220
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
Fixes #24631 - httpboot module #605
Conversation
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.
I haven't tried end-to-end yet, but the proxy module looks good itself. I couldn't get it to do any nefarious traversals using the common tricks, sinatra seems pretty smart.
modules/httpboot/httpboot_api.rb
Outdated
|
||
get "/*" do | ||
file = Pathname.new(params[:splat].first).cleanpath | ||
real_file = Pathname.new(File.join(Proxy::TFTP::Plugin.settings.tftproot, file)).realpath |
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.
Should this be Proxy::Httpboot::Plugin.settings.root_dir
instead?
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.
Also, if you get /EFI or a non-existent file, we return 500. It'd be nice if we return the right http error codes, 404, 503, etc.
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.
Good catch, yeah absolutely 404. Changing.
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.
So I am implementing extra verbose checks just to be sure.
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.
Thanks. Note, that tests are missing.
helpers ::Proxy::Helpers | ||
|
||
get "/*" do | ||
file = Pathname.new(params[:splat].first).cleanpath |
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.
Is this safe to prevent ../../../etc/shadow ?
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.
Sinatra filters it out, among other tricky ways to do path traversal.
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.
Yep I tested this, but I was wondering if we should allow following symlinks outside of TFTP root directory.
I can prevent from both possible security issues by comparing absolute file path with root directory - if it's not the same then we have an error. You know what, let me do that, just for case. I don't want my name on a CVE like this :-)
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.
I am doing this, we can't be sure if e.g. older version of Sinatra does this.
I have amended the last commit, thank you for review. |
Looks good, but a few notes. First, smart Proxy is returning a
This is not valid although most things seem happy to accept it except the edk2 firmware which causes it to freeze. That took a better part of the afternoon to figure out why my vm was locking up. Can you add that to the smart proxy somewhere? |
The other thing is how grub2 doesn't handle relative paths (https://bugzilla.redhat.com/show_bug.cgi?id=1616395). I have a proposed fix to grub2, but even then relative paths (e.g. |
Thanks rebased, it was intentionally set to blank but I explained why we add this back: https://projects.theforeman.org/issues/14394 |
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.
Tested, works great
--- | ||
# Enable publishing of a given directory under /EFI and /httpboot paths. | ||
# Directory listing is not possible, symlinks are followed but not outside | ||
# of the rood directory specified in this file. |
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.
Nit: typoroot
@@ -50,7 +50,7 @@ def http_app(http_port, plugins = http_plugins) | |||
:DoNotListen => true, | |||
:Port => http_port, # only being used to correctly log http port being used | |||
:Logger => ::Proxy::LogBuffer::Decorator.instance, | |||
:ServerSoftware => '', | |||
:ServerSoftware => "foreman-proxy/#{Proxy::VERSION}", |
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.
It's common security practice to not publish the software versions. Do we need this?
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.
It's a common practice in the enterprise, not in the open source world I'd say. But to the point - we do advertise the same via /features
endpoint anyway. And we must not remove this - Foreman now checks foreman version and shows you a warning for compatibility.
If this is a major concern, please file a RM issue to encrypt the version or something like that. But I guess all we can do is obscurity not security in this case.
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.
Ok, fair enough.
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.
I would like to see it as a separate commit. It's not obvious that this is part of the httpboot and I wouldn't look at this commit in git log if I was looking for the change.
# Directory listing is not possible, symlinks are followed but not outside | ||
# of the rood directory specified in this file. | ||
|
||
# Enables the module, make sure to enable TFTP module as well to allow |
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.
You can use requires
in the plugin spec to make sure the tftp module is loaded.
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.
Oh cool this one was exactly what I was looking for. Thanks.
Thanks amended the two things. |
@@ -50,7 +50,7 @@ def http_app(http_port, plugins = http_plugins) | |||
:DoNotListen => true, | |||
:Port => http_port, # only being used to correctly log http port being used | |||
:Logger => ::Proxy::LogBuffer::Decorator.instance, | |||
:ServerSoftware => '', | |||
:ServerSoftware => "foreman-proxy/#{Proxy::VERSION}", |
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.
I would like to see it as a separate commit. It's not obvious that this is part of the httpboot and I wouldn't look at this commit in git log if I was looking for the change.
require 'httpboot/httpboot_plugin' | ||
require 'httpboot/httpboot_api' | ||
|
||
ENV['RACK_ENV'] = 'test' |
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.
It feels weird to set an env var here. Is this needed?
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.
Copy pasted from other file, thought it's required.
I did not want to do this in a separate commit, this might be backported (as it's new module and there cannot be regressions). The feature does not work end-to-end without this. |
How does the code not work without it? Does the client reject empty server strings? |
It would work fine without it (or at least I would guess so), but Sinatra sends I didn't look at the code, but I would guess the HTTP client does a strchr on space or something. |
New module for nicer UEFI HTTP BOOT implementation:
theforeman/foreman#5950
Please pay extra attention to security during review. I tested with paths like
'/EFI/../../../etc/passwd'
or similar. Directory listing is not allowed. Symlinks are followed by design and they are followed even outside of TFTP directory. Not sure if this is a problem, we can limit that tho if needed.