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

HTTP Security fixes #6103

Merged
merged 9 commits into from Feb 21, 2018
Merged

HTTP Security fixes #6103

merged 9 commits into from Feb 21, 2018

Conversation

Crunsher
Copy link
Contributor

This PR addresses a number of security issues with the HTTP server. These changes aim prevent the abuse of the API in a way that causes Icinga 2 to crash.

Refs CVE-2018-6532

@Crunsher Crunsher added this to the 2.8.2 milestone Feb 20, 2018
templates/<type> | /v1/templates | Yes
types | /v1/types | Yes
variables | /v1/variables | Yes
Permissions | URL Endpoint | Supports Filters | Max Body Size in MB
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spelling, either all use "in" or standard English grammar "Max body size in MB". In that case "Support Filters" would need to be changed too.

@@ -27,6 +27,9 @@
# include <poll.h>
#endif /* _WIN32 */

#define TLS_TIMEOUT_SECONDS 10
#define TLS_TIMEOUT_STEP_SECONDS 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unused, remove.

@@ -34,3 +35,31 @@ ApiUser::Ptr ApiUser::GetByClientCN(const String& cn)

return nullptr;
}

ApiUser::Ptr ApiUser::GetByAuthHeader(const String& auth_header)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be fun to merge with the changes to out password system

@@ -62,7 +64,8 @@ struct HttpRequest

HttpRequest(Stream::Ptr stream);

bool Parse(StreamReadContext& src, bool may_wait);
bool ParseHeader(StreamReadContext& src, bool may_wait);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename to "ParseHeaders"

@@ -21,6 +21,7 @@
#define HTTPSERVERCONNECTION_H

#include "remote/httprequest.hpp"
#include "remote/httpresponse.hpp"
#include "remote/apiuser.hpp"
#include "base/tlsstream.hpp"
#include "base/timer.hpp"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This include does not seem to be required here

@Crunsher
Copy link
Contributor Author

Before finishing this up I'd like to hear @dnsmichi and @gunnarbeutner take on the new limits to HTTP body sizes. Here is the table from the docs for easier viewing:

Permissions URL Endpoint Supports filters Max body size in MB
actions/<action> /v1/actions Yes 1
config/query /v1/config No 1
config/modify /v1/config No 512
console /v1/console No 512
events/<type> /v1/events No 1
objects/query/<type> /v1/objects Yes 1
objects/create/<type> /v1/objects No 512
objects/modify/<type> /v1/objects Yes 512
objects/delete/<type> /v1/objects Yes 512
status/query /v1/status Yes 1
templates/<type> /v1/templates Yes 1
types /v1/types Yes 1
variables /v1/variables Yes 1

@dnsmichi
Copy link
Contributor

Mh are these body sizes for requests or responses, or for both?

@Crunsher
Copy link
Contributor Author

Requests, responses are unlimited.

@dnsmichi
Copy link
Contributor

This small detail should be added to the docs then.

The values look good, there's chances to go even lower than these are now.

@Crunsher Crunsher merged commit 184580f into master Feb 21, 2018
{
static const size_t defaultContentLengthLimit = 1 * 1028 * 1028;
static const Dictionary::Ptr specialContentLengthLimits = new Dictionary({
{"*", 512 * 1028 * 1028},
Copy link

Choose a reason for hiding this comment

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

Is there a particular reason why these calculations use 1028 instead of 1 << 10 == 2 ** 10 == 1024?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, we can change that. But is it that much faster?

Copy link

Choose a reason for hiding this comment

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

Oh, I think you misunderstood. It's not about performance (the compiler is likely to do the computation at build time anyway) and instead about power of two being the basis for Mebibyte. 1028 isn't a power of two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh damn, that's a typo > < I'll get that fixed

@Crunsher Crunsher added the backported Fix was included in a bugfix release label Feb 23, 2018
Crunsher added a commit that referenced this pull request Feb 23, 2018
Crunsher added a commit that referenced this pull request Feb 23, 2018
@dnsmichi dnsmichi deleted the fix/http-security-fixes branch April 18, 2018 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Fix was included in a bugfix release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants