Skip to content

isisd: fix router capability TLV parsing issues - #10517

Merged
riw777 merged 1 commit into
FRRouting:masterfrom
idryzhov:isis_router_cap_tlv_fixes
Feb 8, 2022
Merged

isisd: fix router capability TLV parsing issues#10517
riw777 merged 1 commit into
FRRouting:masterfrom
idryzhov:isis_router_cap_tlv_fixes

Conversation

@idryzhov

@idryzhov idryzhov commented Feb 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a rebased version of #9850.
A couple of adjustments made by me:

- fix incorrect replacement of srgb by srlb on lines 3052 and 3054
- add length check for ISIS_SUBTLV_ALGORITHM
- fix conflict in fuzzing data during rebase

@idryzhov idryzhov mentioned this pull request Feb 7, 2022
Closed
whichbug
whichbug reviewed Feb 7, 2022
View reviewed changes
Comment thread isisd/isis_tlvs.c

break;
case ISIS_SUBTLV_ALGORITHM:
/* Only 2 algorithms are supported: SPF & Strict SPF */

@whichbug whichbug Feb 7, 2022 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We may miss a check on length before calling stream_get in this case. We may need a check length != 0 here.

@idryzhov idryzhov Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@NetDEF-CI

NetDEF-CI commented Feb 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator
Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3271/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests Ubuntu 18.04 arm8 part 7: Failed (click for details) Topotests Ubuntu 18.04 arm8 part 7: No useful log found
Topotests Ubuntu 18.04 amd64 part 7: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7U18AMD64-3271/test

Topology Tests failed for Topotests Ubuntu 18.04 amd64 part 7
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-3271/artifact/TOPO7U18AMD64/ErrorLog/log_topotests.txt

Topotests Ubuntu 18.04 i386 part 7: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7U18I386-3271/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 7
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-3271/artifact/TOPO7U18I386/ErrorLog/log_topotests.txt

Topotests debian 10 amd64 part 7: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-PULLREQ2-TOPO7DEB10AMD64-3271/test

Topology Tests failed for Topotests debian 10 amd64 part 7
see full log at https://ci1.netdef.org/browse/FRR-PULLREQ2-3271/artifact/TOPO7DEB10AMD64/ErrorLog/log_topotests.txt

Successful on other platforms/tests
  • Topotests Ubuntu 18.04 i386 part 1
  • Topotests Ubuntu 18.04 i386 part 6
  • Topotests Ubuntu 18.04 amd64 part 4
  • Addresssanitizer topotests part 2
  • Topotests Ubuntu 18.04 arm8 part 6
  • Topotests Ubuntu 18.04 amd64 part 0
  • Topotests Ubuntu 18.04 amd64 part 5
  • Addresssanitizer topotests part 3
  • Topotests Ubuntu 18.04 arm8 part 1
  • Topotests debian 10 amd64 part 6
  • Fedora 29 rpm pkg check
  • Addresssanitizer topotests part 9
  • Topotests debian 10 amd64 part 0
  • Topotests debian 10 amd64 part 5
  • Topotests Ubuntu 18.04 arm8 part 2
  • Topotests Ubuntu 18.04 arm8 part 8
  • CentOS 7 rpm pkg check
  • Addresssanitizer topotests part 6
  • Topotests Ubuntu 18.04 i386 part 2
  • Topotests Ubuntu 18.04 amd64 part 3
  • Topotests debian 10 amd64 part 9
  • Topotests Ubuntu 18.04 amd64 part 2
  • Topotests Ubuntu 18.04 i386 part 5
  • Topotests Ubuntu 18.04 i386 part 0
  • IPv6 protocols on Ubuntu 18.04
  • Addresssanitizer topotests part 0
  • Topotests debian 10 amd64 part 8
  • Debian 10 deb pkg check
  • Topotests debian 10 amd64 part 3
  • Topotests Ubuntu 18.04 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 9
  • Topotests Ubuntu 18.04 arm8 part 4
  • Topotests Ubuntu 18.04 amd64 part 1
  • Ubuntu 18.04 deb pkg check
  • Addresssanitizer topotests part 1
  • Ubuntu 20.04 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 4
  • IPv4 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 amd64 part 9
  • Addresssanitizer topotests part 4
  • Debian 9 deb pkg check
  • Addresssanitizer topotests part 8
  • Topotests Ubuntu 18.04 i386 part 3
  • Topotests Ubuntu 18.04 i386 part 8
  • Addresssanitizer topotests part 7
  • Topotests debian 10 amd64 part 4
  • Topotests Ubuntu 18.04 arm8 part 5
  • Topotests Ubuntu 18.04 arm8 part 3
  • Topotests Ubuntu 18.04 i386 part 9
  • Topotests debian 10 amd64 part 1
  • Addresssanitizer topotests part 5
  • Static analyzer (clang)
  • IPv4 ldp protocol on Ubuntu 18.04
  • Topotests Ubuntu 18.04 arm8 part 0
  • Ubuntu 16.04 deb pkg check
  • Topotests Ubuntu 18.04 amd64 part 8
  • Topotests debian 10 amd64 part 2

@jvijtiuk @idryzhov
isisd: fix router capability TLV parsing issues
9ba865f 
isis_tlvs.c would fail at multiple places if incorrect TLVs were
received causing stream assertion violations.
This patch fixes the issues by adding missing length checks, missing
consumed length updates and handling malformed Segment Routing subTLVs.

Signed-off-by: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr>

Small adjustments by Igor Ryzhov:
- fix incorrect replacement of srgb by srlb on lines 3052 and 3054
- add length check for ISIS_SUBTLV_ALGORITHM
- fix conflict in fuzzing data during rebase

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
@idryzhov
idryzhov force-pushed the isis_router_cap_tlv_fixes branch from 21ba3e0 to 9ba865f Compare February 8, 2022 08:32
@idryzhov

idryzhov commented Feb 8, 2022

Copy link
Copy Markdown
Contributor Author

I finally found and fixed the issue that caused CI failures. Added the following to the commit message:

Small adjustments by Igor Ryzhov:
- fix incorrect replacement of srgb by srlb on lines 3052 and 3054
- add length check for ISIS_SUBTLV_ALGORITHM
- fix conflict in fuzzing data during rebase

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>

@NetDEF-CI

NetDEF-CI commented Feb 8, 2022

Copy link
Copy Markdown
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-PULLREQ2-3284/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

riw777
riw777 approved these changes Feb 8, 2022
View reviewed changes

@riw777 riw777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

looks good

@riw777
riw777 merged commit 107f77b into FRRouting:master Feb 8, 2022
@idryzhov idryzhov mentioned this pull request Feb 8, 2022
Closed
@idryzhov

idryzhov commented Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

@Mergifyio backport dev/8.2

@FRRouting FRRouting deleted a comment from mergify Bot Feb 9, 2022
@mergify mergify Bot mentioned this pull request Feb 9, 2022
Merged
@mergify

mergify Bot commented Feb 9, 2022

Copy link
Copy Markdown

backport dev/8.2

✅ Backports have been created

Details

donaldsharp added a commit that referenced this pull request Feb 9, 2022
@donaldsharp
Merge pull request #10542 from FRRouting/mergify/bp/dev/8.2/pr-10517
6110c81 
isisd: fix router capability TLV parsing issues (backport #10517)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@riw777 riw777 riw777 approved these changes
+1 more reviewer
@whichbug whichbug whichbug left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugfix isis

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@idryzhov @NetDEF-CI @riw777 @whichbug @jvijtiuk