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

Parser the dyn_port when in FTP active mode #3302

Closed
wants to merge 1 commit into from

Conversation

zhangqunshi
Copy link

@zhangqunshi zhangqunshi commented Mar 21, 2018

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:

Describe changes:

  • When the FTP is active mode, the PORT command will contain the dyn_port, but current code doesn't parse it. So add a new method FTPParseGetDynPort to handler dyn_port, and call it in the case FTP_COMMAND_PORT.

PRScript output (if applicable):

…rt, but current code doesn't parse it.

So add a new method FTPParseGetDynPort to handler dyn_port, and call it in the case FTP_COMMAND_PORT.
@zhangqunshi zhangqunshi requested a review from a team as a code owner March 21, 2018 09:39
@victorjulien
Copy link
Member

Hi, please have a look at the contribution agreement. If you have any questions or concerns contact me by email at vjulien@oisf.net

@zhangqunshi
Copy link
Author

I have sent the mail to you, please check inbox.

Copy link
Contributor

@regit regit left a comment

Choose a reason for hiding this comment

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

Rust part is not working. Update is required.

@@ -425,6 +453,8 @@ static int FTPParseRequest(Flow *f, void *ftp_state,
memcpy(state->port_line, state->current_line,
state->current_line_len);
state->port_line_len = state->current_line_len;
// TODO need to handle ipv6
FTPParseGetDynPort(state, input, input_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't you call directly FTPParsePassiveResponse ? The pointer to the Flow is also available here available. At worse if the parsing is the same, you can rename the function.

Copy link
Author

Choose a reason for hiding this comment

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

Because I hope the method FTPParseGetDynPort can be used in FTP active and passive both. And sorry, i don't know rust, so i don't know how to handle it.

@@ -1328,4 +1335,3 @@ void FTPParserRegisterTests(void)
UtRegisterTest("FTPParserTest10", FTPParserTest10);
#endif /* UNITTESTS */
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, no whitespace fixes in a functional commit.

Copy link
Author

Choose a reason for hiding this comment

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

OK, but i didn't add it, maybe my IDE did it.

uint16_t dyn_port;

#ifdef HAVE_RUST
dyn_port = rs_ftp_pasv_response(input, input_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work in rust because the rust parsing function is looking for something starting with 227 response code.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, i don't know rust, so can anybody help me to add a method rs_ftp_active_response in the rust file?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can skip the rust code and use some C code only if you refactor a bit. But rust is not that complicated if you have example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants