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

gnrc_tftp: initialize unititialized 'tftp_context_t' #11773

Merged
merged 4 commits into from
Jul 3, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 1, 2019

Contribution description

Fixes #11772.

Testing procedure

Follow procedure in #11772. It should succeed and also not run into a failed assertion. Also try the instruction in example/gnrc_tftp's README.

Issues/PRs references

Addresses #11772.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2019
@miri64 miri64 added this to the Release 2019.07 milestone Jul 1, 2019
@miri64 miri64 changed the title Gnrc tftp/fix/uninitialized values gnrc_tftp: initialize unititialized 'tftp_context_t' Jul 1, 2019
@@ -606,6 +608,7 @@ tftp_state _tftp_state_processes(tftp_context_t *ctxt, msg_t *m)
tmp = gnrc_pktsnip_search_type(pkt, GNRC_NETTYPE_IPV6);
ipv6_hdr_t *ip = (ipv6_hdr_t *)tmp->data;
uint8_t *data = (uint8_t *)pkt->data;
ctxt->dst_port = byteorder_ntohs(udp->src_port);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure about this one. There are also other sets of this variable below... However, I observed gnrc_udp_hdr_build() getting dst_port 0 with the fix above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a fixup that I think is more correct.

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

For the testing procedure I needed to run the following echo AAMAAGZvb2Jhcg== | base64 -d | nc -6 -u 'fe80::680d:c6ff:fed2:1f3a%tapbr0' 69
I was able to fail it on master and this PR does fix it.

It looks good generally. Please squash.

@MrKevinWeiss MrKevinWeiss added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jul 3, 2019
ctxt.enable_options = use_options;
tftp_context_t ctxt = {
.dst_port = GNRC_TFTP_DEFAULT_DST_PORT,
.src_port = GNRC_TFTP_DEFAULT_DST_PORT,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure this should be SRC_PORT... I'd like to do some personal testing for the "normal" operation first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let me how to confirm then in the test procedure (or ping someone more knowledgeable).

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the contributor that provided this is not active anymore, I'd say follow the README. Maybe also @bergzand has some input.

Copy link
Member

Choose a reason for hiding this comment

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

Assuming my understanding of the protocol and the implementation here is sufficient, this .src_port is the UDP port used as source by RIOT and used as destination by the peer. Both should be ephermal ports specific for this connection.

Does it make sense to default it to something invalid/unconfigured? (Do we have something like that in gnrc?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Assuming my understanding of the protocol and the implementation here is sufficient, this .src_port is the UDP port used as source by RIOT and used as destination by the peer. Both should be ephermal ports specific for this connection.

We are in server context here, so src_port should at least be GNRC_TFTP_DEFAULT_DST_PORT (which is the default TFTP port). Ephemeral ports in GNRC are part of the gnrc_sock implementation, which this implementation does not use :-/

Does it make sense to default it to something invalid/unconfigured? (Do we have something like that in gnrc?)

If I leave it unconfigured, the testing procedure in #11772 runs into an assertion here

gnrc_pktsnip_t *gnrc_udp_hdr_build(gnrc_pktsnip_t *payload, uint16_t src,
uint16_t dst)
{
assert((src > 0) && (dst > 0));

from the call here

src_port.u16 = ctxt->src_port;
dst_port.u16 = ctxt->dst_port;
udp = gnrc_udp_hdr_build(buf, src_port.u16, dst_port.u16);

This is the only application protocol purely based on GNRC (and as soon as we have async sock, I prefer to have it ported to sock_udp, but first we need to fix the bug ;-)).

Copy link
Member

@nmeum nmeum Jul 3, 2019

Choose a reason for hiding this comment

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

If I leave it unconfigured, the testing procedure in #11772 runs into an assertion here […]

You did leave it (dst_port) unconfigered in the merged version of your PR, right? Because doesn't this allow someone to crash a RIOT node running gnrc_tftp compiled with DEVELHELP = 1 by sending a crafted input?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arghs, this must have gotten lost, when I squashed :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait no, right. I moved the initialization to 2fce095, as _tftp_state_processes relied on certain configurations of ctxt->dst_port I was unsure about. I tested it with your crafted input, and it did not crash (the server was not responding anymore, because a legal input changed its port...., see #11778).

Copy link
Member

@nmeum nmeum Jul 3, 2019

Choose a reason for hiding this comment

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

I tested it with your crafted input, and it did not crash

My previous test input gets rejected due to the blocknum contained in it. If you compile with ENABLE_DEBUG you get (among other things):

tftp: not the packet we were waiting for, expected 1, received 0

If you change the blocknum in the test input gnrc_tftp crashes:

echo AAMAAWZvb2Jhcg | base64 -d | nc -u '[ip-address%tap0]' 69

Can you reproduces this or am I simply doing something wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp... Why did I touch this module -.- It's good that I deprecated it.

@miri64
Copy link
Member Author

miri64 commented Jul 3, 2019

I just noticed, that the server does not response ever since the shift to gnrc_netif (same old problem: link-local address without a given interface). I'll try and see if I can at least fix it for the server (the client would require an API change).

@miri64 miri64 force-pushed the gnrc_tftp/fix/uninitialized-values branch from b79aa36 to 2fce095 Compare July 3, 2019 10:51
@miri64
Copy link
Member Author

miri64 commented Jul 3, 2019

I can at least be sure with the latest squash, that the assertion I ran into is avoided.

@miri64
Copy link
Member Author

miri64 commented Jul 3, 2019

Ok. I added a fix for the link-local address problem I mentioned before, but now I see, that with the fix I can still cause the server to switch source port when I send @nmeum's test message :-/....

@miri64
Copy link
Member Author

miri64 commented Jul 3, 2019

Ok. It is not the test message causing the port to switch, but the normal message. I'm for merging these fixes soonish and then deprecating the module, as it is obviously flawed and unused.

@MrKevinWeiss MrKevinWeiss removed Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jul 3, 2019
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I retested. The fix still works. The docs make sense to me. Since this module will be deprecated soon anyways I think it will it would be good to have the fix in.

ACK.

@MrKevinWeiss MrKevinWeiss added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 3, 2019
@miri64 miri64 merged commit e8650f5 into RIOT-OS:master Jul 3, 2019
@miri64 miri64 deleted the gnrc_tftp/fix/uninitialized-values branch July 3, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gnrc_tftp: use of uninitialized values
4 participants