micro_http: check arithmetic operations #1998

ioanachirca · 2020-07-03T11:47:31Z

Signed-off-by: Ioana Chirca chioana@amazon.com

Reason for This PR

Part of #2003
This PR adds checks/comments for the micro_http operations that are not fixed in #1985

Description of Changes

[Author TODO: add description of changes.]

This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

src/micro_http/src/request.rs

andreeaflorescu · 2020-07-06T10:51:27Z

src/micro_http/src/request.rs

-            let uri_and_version = &request_line[(method_end + 1)..];
+            let uri_start = method_end.checked_add(1).ok_or(RequestError::Overflow)?;
+
+            // Slice access is safe because `uri_start` <= `request_line` size.


Who validates that uri_start is <= request_line? The above check seem to only check that the integer does not overflow.

+1 on this. The comments in this function should be clearer and point to the exact place where the check is assured (in this case, find call).

Also, it looks like, in the following scenario:

let request_line = b"GET"; assert_eq!( RequestLine::try_from(request_line).unwrap_err(), RequestError::InvalidHttpMethod("Unsupported HTTP method.") );

the returned error is Unsupported HTTP method, but I don't know/think that this error is the best one, seems really confusing, would it be better to return something else?
Maybe parse_request_line() could return an error if SP is not found, what do you think?

I added more comments, hope it got a littler clearer.
Also modified parse_request_line to return an error if there is no SP.

andreeaflorescu · 2020-07-06T10:54:43Z

src/micro_http/src/request.rs

-                let version = &uri_and_version[(uri_end + 1)..];
+                let version_start = uri_end.checked_add(1).ok_or(RequestError::Overflow)?;
+
+                // Slice access is safe because `version_start` <= `uri_and_version` size.


Same question as above.

Tried to add comments. 😅

lauralt · 2020-07-06T11:42:30Z

src/micro_http/src/request.rs

-            let uri_and_version = &request_line[(method_end + 1)..];
+            let uri_start = method_end.checked_add(1).ok_or(RequestError::Overflow)?;
+
+            // Slice access is safe because `uri_start` <= `request_line` size.


+1 on this. The comments in this function should be clearer and point to the exact place where the check is assured (in this case, find call).

Also, it looks like, in the following scenario:

let request_line = b"GET"; assert_eq!( RequestLine::try_from(request_line).unwrap_err(), RequestError::InvalidHttpMethod("Unsupported HTTP method.") );

the returned error is Unsupported HTTP method, but I don't know/think that this error is the best one, seems really confusing, would it be better to return something else?
Maybe parse_request_line() could return an error if SP is not found, what do you think?

lauralt · 2020-07-06T11:52:49Z

src/micro_http/src/request.rs

+                        let body_len = headers_and_body
+                            .len()
+                            .checked_sub(crlf_end)
+                            .ok_or(RequestError::Underflow)?;


Is it possible to have tests for at least some of the Underflow, Overflow scenarios that are handled in this PR?

The underflows don't seem possible so I removed them and added comments. for overflows it's a little harder to test 😞

src/micro_http/src/common/mod.rs

lauralt · 2020-07-06T11:58:19Z

src/micro_http/src/request.rs

                            return Err(RequestError::InvalidRequest);
                        }
-                        let body_as_bytes = &headers_and_body[(headers_end + 2 * CRLF_LEN)..];
+                        let body_as_bytes = &headers_and_body[crlf_end..];


This one might deserve a comment too.

src/micro_http/src/common/mod.rs

src/micro_http/src/request.rs

lauralt · 2020-07-13T09:01:42Z

src/dumbo/src/tcp/endpoint.rs

+            RequestError::Underflow => build_response(
+                Version::default(),
+                StatusCode::BadRequest,
+                Body::new(e.to_string()),
+            ),


nit: since this seems unreachable now, I would move this arm at the end of this match, right before the UnsupportedHeader one (I wouldn't though replace build_response() with unreachable!()since maybe try_from() eventually will be able to return Underflow too and it would be pretty easy to forget updating here with it).

lauralt · 2020-07-13T09:06:38Z

src/micro_http/src/request.rs

        }

-        (b"", b"", b"")
+        Err(RequestError::InvalidRequest)


nit: maybe add a comment here that we now consider as invalid a request with no SPs.

Added. Also removed the case when we return an Ok when there is no HTTP version.

lauralt · 2020-07-13T09:15:43Z

src/micro_http/src/request.rs

+                // Underflow is not possible here because `byte_stream[request_line_end..]` starts with CR LF,
+                // so `headers_end` can be either zero (first match arm) or >= 3.


nit (to avoid any kind of confusion):

Suggested change

// Underflow is not possible here because `byte_stream[request_line_end..]` starts with CR LF,

// so `headers_end` can be either zero (first match arm) or >= 3.

// Underflow is not possible here because `byte_stream[request_line_end..]` starts with CR LF,

// so `headers_end` can be either zero (but this case is treated separately in the first match arm)

// or >= 3 (current case).

Signed-off-by: Ioana Chirca <chioana@amazon.com>

ioanachirca self-assigned this Jul 3, 2020

andreeaflorescu reviewed Jul 6, 2020

View reviewed changes

lauralt reviewed Jul 6, 2020

View reviewed changes

andreeaflorescu reviewed Jul 6, 2020

View reviewed changes

src/micro_http/src/request.rs Outdated Show resolved Hide resolved

ioanachirca force-pushed the arithmetic_ops branch 2 times, most recently from c92bebf to 7bc15d8 Compare July 7, 2020 08:19

lauralt previously approved these changes Jul 13, 2020

View reviewed changes

ioanachirca dismissed lauralt’s stale review via 9b9bbdc July 13, 2020 11:28

ioanachirca force-pushed the arithmetic_ops branch 2 times, most recently from 9b9bbdc to 5878780 Compare July 13, 2020 11:31

andreeaflorescu previously approved these changes Jul 13, 2020

View reviewed changes

ioanachirca dismissed andreeaflorescu’s stale review via 97e77c0 July 13, 2020 15:45

ioanachirca force-pushed the arithmetic_ops branch 2 times, most recently from 97e77c0 to 9d8be78 Compare July 14, 2020 06:43

micro_http: check arithmetic operations

b6581d3

Signed-off-by: Ioana Chirca <chioana@amazon.com>

ioanachirca force-pushed the arithmetic_ops branch from 9d8be78 to b6581d3 Compare July 14, 2020 07:10

lauralt approved these changes Jul 14, 2020

View reviewed changes

andreeaflorescu approved these changes Jul 14, 2020

View reviewed changes

ioanachirca merged commit c8e5bcc into firecracker-microvm:master Jul 14, 2020

ioanachirca deleted the arithmetic_ops branch July 14, 2020 08:28

alxiord mentioned this pull request Jul 23, 2020

micro-http: check arithmetic operations #2003

Closed

		// Underflow is not possible here because `byte_stream[request_line_end..]` starts with CR LF,
		// so `headers_end` can be either zero (first match arm) or >= 3.

micro_http: check arithmetic operations #1998

micro_http: check arithmetic operations #1998

Uh oh!

Conversation

ioanachirca commented Jul 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ioanachirca commented Jul 3, 2020 •

edited

Loading