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

Warp: don't send the Server header by default #619

Closed
Gothdo opened this Issue Apr 20, 2017 · 28 comments

Comments

Projects
None yet
4 participants
@Gothdo
Copy link

Gothdo commented Apr 20, 2017

I don't think there's any good reason to send the Server header, and in fact it can even be harmful. RFC 2068 says:

Revealing the specific software version of the server may allow the server machine to become more vulnerable to attacks against software that is known to contain security holes. Implementers SHOULD make the Server header field a configurable option.

See also this article: Shhh… don’t let your response headers talk too loudly.

Can you make it that Warp doesn't send the Server header by default, or at least add an option to disable that?

@psibi

This comment has been minimized.

Copy link
Member

psibi commented Apr 20, 2017

I don't think Security through obscurity is good...

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented Apr 20, 2017

@psibi But that doesn't mean that you should give a potential attacker any information that could help them in exploiting your server. The article I linked says:

It’s not about making a web app “secure” as if it’s one practice to rule them all, no, it’s about keeping information which could be used as part of a broader attack a little quiet.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented Apr 21, 2017

It is easy to implement such an option. We should make a consensus on what kind of options should be implemented. For instance,

  • Reuse setServerName. If it set "", Server: is not sent
  • Define setServerNameDisabled.
@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented Apr 21, 2017

@kazu-yamamoto I think the first option is better, because it doesn't make sense to send an empty "Server" header anyway.

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 1, 2017

@psibi @kazu-yamamoto Any update on this?

kazu-yamamoto added a commit that referenced this issue May 2, 2017

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 2, 2017

Sorry for the delay. Please give a try to the patch above.

kazu-yamamoto added a commit that referenced this issue May 2, 2017

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 2, 2017

Please try the master branch. The first one is on a wrong branch, sorry.

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 2, 2017

@kazu-yamamoto I added the patched package to my stack.yaml file:

packages:
- '.'
- location:
    git: git@github.com:yesodweb/wai.git
    commit: 9fd0dcf221e4946d3588eea3ccd87492ca718d6a
  extra-dep: true
  subdirs:
    - warp

and I added setServerName "" to my code but it still doesn't work. The server sends header Server: Warp/3.2.11.1, but the version of Warp is 3.2.11.2. Maybe this is because I'm using Yesod, and Yesod has Warp as a dependency. How can I make Yesod use the patched package?

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 3, 2017

What's the output of this command?

stack exec -- ghc-pkg list warp
@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 3, 2017

@kazu-yamamoto I think the current implementation does not work as expected. I got the following change to produce the expected results, but it's likely not as efficient as desired:

 addServer :: HeaderValue -> IndexedHeader -> H.ResponseHeaders -> H.ResponseHeaders
 addServer serverName rspidxhdr hdrs = case rspidxhdr ! fromEnum ResServer of
-    Nothing
-      | serverName /= "" -> (H.hServer, serverName) : hdrs
-    _                    -> hdrs
+    Nothing                -> (H.hServer, serverName) : hdrs
+    Just serverName'
+      | S.null serverName' -> filter (\(x, _) -> x /= "Server") hdrs
+      | otherwise          -> hdrs
@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 3, 2017

@snoyberg When I run this command, I get the following output:

/home/michal/.stack/programs/x86_64-linux/ghc-8.0.2/lib/ghc-8.0.2/package.conf.d
    (no packages)
/home/michal/.stack/snapshots/x86_64-linux/lts-8.8/8.0.2/pkgdb
    warp-3.2.11.1
/home/michal/backend/.stack-work/install/x86_64-linux/lts-8.8/8.0.2/pkgdb
    warp-3.2.11.2

/home/michal/backend/ is the directory of my project (and my current working directory).

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 3, 2017

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 3, 2017

@snoyberg But why the Warp version in the Server header is 3.2.11.1 instead of 3.2.11.2?

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 3, 2017

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 3, 2017

@snoyberg Yes, I'm using yesod devel.

I reinstalled all packages, and now when I run the server with yesod devel, I get Server: Warp/3.2.11.2, but when I run the executable built by stack, there's no Server header, so the patch is working.

I think this issue can be closed now.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 8, 2017

@snoyberg Sorry for the delay. I was on vacation.
And sorry for my careless coding. Your patch looks good to me.
Would you push your patch to the master branch?
Please don't forget to modify HTTP2/HPACK, too.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 8, 2017

Actually, I'm a bit confused now. @Gothdo is reporting that the patch does appear to work, though it was not working for me. Reviewing the code, it does not look like it should work. I think I'll go ahead with pushing to master, but @Gothdo, if you could check again that it really is working as expected, I'd appreciate it.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 8, 2017

@kazu-yamamoto Reviewing the HPACK code, I think it's correct already.

snoyberg added a commit that referenced this issue May 8, 2017

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 8, 2017

@snoyberg Yes, it's working as expected.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 8, 2017

Can you tell me if you're connecting over HTTP 2?

@Gothdo

This comment has been minimized.

Copy link
Author

Gothdo commented May 8, 2017

@snoyberg No, I'm using HTTP 1.1.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 9, 2017

I believe:

  • Both @snoyberg and @Gothdo are using HTTP/1.1
  • @snoyberg's application calls sendResponse with Server: included. So, my patch does not delete Server:.
  • @Gothdo's application calls sendResponse without Server:. So, my patch does not add an additional Server:.

Make sense?

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 9, 2017

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 9, 2017

Sounds like a good hypothesis.

Good.

Reviewing the HPACK code, I think it's correct already.

Uhhm. I think that Server: should be deleted if exists. I will check.

kazu-yamamoto added a commit that referenced this issue May 9, 2017

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 9, 2017

@snoyberg Before tackling HTTP/2, I added test case for HTTP/1.1.
To my surprise, your code is still buggy. The patch above should fix the bug.
Please review this patch.
c4000b0 explains the expected behavior.

@snoyberg

This comment has been minimized.

Copy link
Member

snoyberg commented May 9, 2017

👍

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 9, 2017

The HTTP/2 part is also done.
Let's close this.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor

kazu-yamamoto commented May 11, 2017

I have released a new version of Warp.

kazu-yamamoto added a commit to kazu-yamamoto/wai that referenced this issue Sep 7, 2018

fixing a bug of addHeader.
See 78022f9
"test cases for addHeader and fix (yesodweb#619)"
This commit contains a terrible bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.