pimd: IGMPv3 conformance fixes for invalid TTL and invalid TOS #9074
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
polychaeta
reviewed
Jul 16, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/7b7937e64ed8b04c2d15b8859066d1c6/raw/af604b851750f9d4d413076c6aae1f74d09df948/cr_9074_1626437543.diff | git apply
diff --git a/pimd/pim_igmp.c b/pimd/pim_igmp.c
index 7e82ee4f1..46848a0c1 100644
--- a/pimd/pim_igmp.c
+++ b/pimd/pim_igmp.c
@@ -490,7 +490,7 @@ int pim_igmp_verify_header(struct ip *ip_hdr, size_t len, int igmp_msg_len,
zlog_warn(
"Recv IGMP packet with invalid ttl=%u, discarding the packet",
ip_hdr->ip_ttl);
- return -1;
+ return -1;
}
}
@@ -545,9 +545,9 @@ int pim_igmp_packet(struct igmp_sock *igmp, char *buf, size_t len)
if (pim_igmp_verify_header(ip_hdr, len, igmp_msg_len, msg_type) == -1) {
zlog_warn(
- "Recv IGMP packet from %s to %s on %s: size=%zu ttl=%u msg_type=%d msg_size=%d",
- from_str, to_str, igmp->interface->name, len,
- ip_hdr->ip_ttl, msg_type, igmp_msg_len);
+ "Recv IGMP packet from %s to %s on %s: size=%zu ttl=%u msg_type=%d msg_size=%d",
+ from_str, to_str, igmp->interface->name, len,
+ ip_hdr->ip_ttl, msg_type, igmp_msg_len);
return -1;
}
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
|
ci:rerun |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 9: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 9: No useful log foundTopotests debian 10 amd64 part 5: Failed (click for details)Topotests debian 10 amd64 part 5: No useful log foundTopotests debian 10 amd64 part 0: Failed (click for details)Topotests debian 10 amd64 part 0: No useful log foundTopotests Ubuntu 18.04 i386 part 2: Failed (click for details)Topotests Ubuntu 18.04 i386 part 2: No useful log foundTopotests Ubuntu 18.04 i386 part 3: Failed (click for details)Topotests Ubuntu 18.04 i386 part 3: No useful log foundTopotests debian 10 amd64 part 1: Failed (click for details)Topotests debian 10 amd64 part 1: No useful log foundTopotests Ubuntu 18.04 i386 part 7: Failed (click for details)Topotests Ubuntu 18.04 i386 part 7: No useful log foundTopotests Ubuntu 18.04 amd64 part 0: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 0: No useful log foundTopotests Ubuntu 18.04 amd64 part 2: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 2: No useful log foundTopotests Ubuntu 18.04 i386 part 4: Failed (click for details)Topotests Ubuntu 18.04 i386 part 4: No useful log foundTopotests Ubuntu 18.04 i386 part 1: Failed (click for details)Topotests Ubuntu 18.04 i386 part 1: No useful log foundTopotests Ubuntu 18.04 i386 part 6: Failed (click for details)Topotests Ubuntu 18.04 i386 part 6: No useful log foundTopotests Ubuntu 18.04 arm8 part 8: Failed (click for details)Topotests Ubuntu 18.04 arm8 part 8: No useful log foundTopotests debian 10 amd64 part 2: Failed (click for details)Topotests debian 10 amd64 part 2: No useful log foundTopotests Ubuntu 18.04 amd64 part 6: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 6: No useful log foundTopotests debian 10 amd64 part 4: Failed (click for details)Topotests debian 10 amd64 part 4: No useful log foundTopotests Ubuntu 18.04 amd64 part 1: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 1: No useful log foundTopotests Ubuntu 18.04 i386 part 5: Failed (click for details)Topotests Ubuntu 18.04 i386 part 5: No useful log foundTopotests debian 10 amd64 part 8: Failed (click for details)Topotests debian 10 amd64 part 8: No useful log foundTopotests Ubuntu 18.04 amd64 part 5: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 5: No useful log foundTopotests Ubuntu 18.04 amd64 part 3: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 3: No useful log foundTopotests Ubuntu 18.04 amd64 part 4: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 4: No useful log foundTopotests Ubuntu 18.04 amd64 part 7: Failed (click for details)Topotests Ubuntu 18.04 amd64 part 7: No useful log foundTopotests Ubuntu 18.04 i386 part 0: Failed (click for details)Topotests Ubuntu 18.04 i386 part 0: No useful log foundTopotests debian 10 amd64 part 9: Failed (click for details)Topotests debian 10 amd64 part 9: No useful log foundSuccessful on other platforms/tests
|
donaldsharp
requested changes
Jul 19, 2021
polychaeta
previously requested changes
Jul 19, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
Click for style suggestions
To apply these suggestions:
curl -s https://gist.githubusercontent.com/polychaeta/422fa04b5b9d9b911948484487a02010/raw/53acfaeabb7f79b94fa04116fe1316c81df41ae0/cr_9074_1626723289.diff | git apply
diff --git a/pimd/pim_igmp.c b/pimd/pim_igmp.c
index 0f0283776..477cf991b 100644
--- a/pimd/pim_igmp.c
+++ b/pimd/pim_igmp.c
@@ -470,7 +470,7 @@ static int igmp_v1_recv_report(struct igmp_sock *igmp, struct in_addr from,
}
bool pim_igmp_verify_header(struct ip *ip_hdr, size_t len, int igmp_msg_len,
- int msg_type)
+ int msg_type)
{
if (len < sizeof(*ip_hdr)) {
zlog_warn("IGMP packet size=%zu shorter than minimum=%zu", len,
diff --git a/pimd/pim_igmp.h b/pimd/pim_igmp.h
index 4ad49f9d3..88324b793 100644
--- a/pimd/pim_igmp.h
+++ b/pimd/pim_igmp.h
@@ -117,7 +117,7 @@ void igmp_sock_free(struct igmp_sock *igmp);
void igmp_sock_delete_all(struct interface *ifp);
int pim_igmp_packet(struct igmp_sock *igmp, char *buf, size_t len);
bool pim_igmp_verify_header(struct ip *ip_hdr, size_t len, int igmp_msg_len,
- int msg_type);
+ int msg_type);
void pim_igmp_general_query_on(struct igmp_sock *igmp);
void pim_igmp_general_query_off(struct igmp_sock *igmp);
void pim_igmp_other_querier_timer_on(struct igmp_sock *igmp);
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
polychaeta
previously requested changes
Jul 19, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
polychaeta
previously requested changes
Jul 19, 2021
There was a problem hiding this comment.
Thanks for your contribution to FRR!
- One of your commits has a missing or badly formatted
Signed-off-byline; we can't accept your contribution until all of your commits have one
If you are a new contributor to FRR, please see our contributing guidelines.
After making changes, you do not need to create a new PR. You should perform an amend or interactive rebase followed by a force push.
Moving the header verification checks inside a function. Signed-off-by: Mobashshera Rasool <mrassol@vmware.com>
IGMPv3 packets with invalid TTL should be dropped. Test Case ID: 4.10 TEST_DESCRIPTION Every IGMP message described in this document is sent with an IP Time-to-Live of 1 (Tests that IGMPv3 Membership Report Message conforms to above statement) TEST_REFERENCE NEGATIVE: RFC 3376, IGMP Version 3, s4 p7 Message Formats Issue: FRRouting#9070 Signed-off-by: Mobashshera Rasool <mrasool@vmware.com>
IGMPv3 packets with invalid TOS should be dropped. Test Case ID: 4.10 TEST_DESCRIPTION Every IGMP message described in this document is sent with IP Precedence of Internetwork Control (e.g., Type of Service 0xc0) (Tests that IGMPv3 Membership Query Message conforms to above statement) TEST_REFERENCE NEGATIVE: RFC 3376, IGMP Version 3, s4 p7 Message Formats Issue: FRRouting#9071 Signed-off-by: Mobashshera Rasool <mrasool@vmware.com>
Outdated results 💚Basic BGPD CI results: SUCCESS, 0 tests failed
For details, please contact louberger |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20360/ This is a comment from an automated CI system. |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20361/ This is a comment from an automated CI system. |
Continuous Integration Result: SUCCESSFULContinuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20362/ This is a comment from an automated CI system. |
Continuous Integration Result: FAILEDContinuous Integration Result: FAILEDSee below for issues. This is a comment from an automated CI system. Get source / Pull Request: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopotests Ubuntu 18.04 amd64 part 1: Failed (click for details)Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TP1U1804AMD64-20363/test Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 1: see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20363/artifact/TP1U1804AMD64/ErrorLog/log_topotests.txt Successful on other platforms/tests
|
|
ci:rerun |
Fixed it. |
Continuous Integration Result: SUCCESSFULCongratulations, this patch passed basic tests Tested-by: NetDEF / OpenSourceRouting.org CI System CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-20380/ This is a comment from an automated CI system. |
idryzhov
reviewed
Jul 23, 2021
| @@ -479,17 +518,8 @@ int pim_igmp_packet(struct igmp_sock *igmp, char *buf, size_t len) | |||
| char from_str[INET_ADDRSTRLEN]; | |||
| char to_str[INET_ADDRSTRLEN]; | |||
|
|
|||
| if (len < sizeof(*ip_hdr)) { | |||
There was a problem hiding this comment.
It's not correct to move this check to pim_igmp_verify_header.
You're using header fields before the call to this function.
Please, move it back.
There was a problem hiding this comment.
@mobash-rasool please fix this and the next comment.
There was a problem hiding this comment.
Okay sure, will check and raise a new PR.
idryzhov
reviewed
Jul 23, 2021
| @@ -501,22 +531,26 @@ int pim_igmp_packet(struct igmp_sock *igmp, char *buf, size_t len) | |||
|
|
|||
| igmp_msg = buf + ip_hlen; | |||
| igmp_msg_len = len - ip_hlen; | |||
|
|
|||
| if (igmp_msg_len < PIM_IGMP_MIN_LEN) { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
IGMPv3 packets with invalid TTL should be dropped.
Test Case ID: 4.10
TEST_DESCRIPTION
Every IGMP message described in this document is sent with an IP
Time-to-Live of 1 (Tests that IGMPv3 Membership Report Message
conforms to above statement)
TEST_REFERENCE
NEGATIVE: RFC 3376, IGMP Version 3, s4 p7 Message Formats
Issue: FRR accept IGMPv3 report sent with invalid TTL #9070
IGMPv3 packets with invalid TOS should be dropped.
Test Case ID: 4.10
TEST_DESCRIPTION
Every IGMP message described in this document is sent with
IP Precedence of Internetwork Control (e.g., Type of Service
0xc0)
(Tests that IGMPv3 Membership Query Message conforms to
above statement)
TEST_REFERENCE
NEGATIVE: RFC 3376, IGMP Version 3, s4 p7 Message Formats
Issue: IGMPv3 report sent in response to invalid TOS query #9071
Signed-off-by: Mobashshera Rasool mrasool@vmware.com