ospf6d: fix heap use after free #1217
Conversation
qlyoung
force-pushed
the
fix-heap-uaf-ospf6
branch
from
b9dfef0 to
df6cde4
Compare
Continuous Integration Result: FAILEDSee below for issues. This is a comment from an EXPERIMENTAL automated CI system. Get source and apply patch from patchwork: SuccessfulBuilding Stage: SuccessfulBasic Tests: FailedTopology tests on Ubuntu 16.04 amd64: Successful IPv4 protocols on Ubuntu 14.04: FailedRFC Compliance Test ANVL-BGP4-15.28 failing: CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
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-1731/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
false positives |
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-1732/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
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-1734/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
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-1733/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
@qlyoung can you make sure you run ospf-smoke with this change? |
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-1735/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
Rerunning tests - Address Sanitizer Tests were disabled for some reason |
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-1736/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
I still see many heap-after-free Address Sanitizer errors: |
|
@mwinter-osr check the line numbers, you are testing master and not stable/3.0. |
|
Yes, something went wrong... troubleshooting the CI - ignore the next CI reruns |
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-1738/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
Ok, address sanitizer fixed. There was a bad bug where leftover errors from previous results got reported. Issue is fixed. Your PR correctly fixes the issue. |
During the loop we save a pointer to the next route in the table in case brouter is deleted during the course of the loop iteration. However when we call ospf6_route_remove this can trigger ospf6_route_remove on other routes in the table, one of which could be pointed at by said pointer. Since ospf6_route_next locks the route that it returns, it won't actually be deleted, instead the refcount will go to 1. In the next loop iteration, nbrouter becomes brouter, and calling ospf6_route_next on this one will finally decrement the refcount to 0, resulting in a free, which causes subsequent reads on brouter to be UAF. Since the route will have OSPF6_ROUTE_WAS_REMOVED set, provided the memory was not overwritten before we got there, we'll continue on to the next one so it is unlikely this will cause a crash in production. Solution implemented is to check if we've deleted the route and continue if so. Signed-off-by: Quentin Young <qlyoung@cumulusnetworks.com>
qlyoung
force-pushed
the
fix-heap-uaf-ospf6
branch
from
df6cde4 to
e2ee000
Compare
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-1739/ This is a comment from an EXPERIMENTAL automated CI system. CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base138 Static Analyzer issues remaining.See details at |
|
just some style nits |
| } else { | ||
| nbrouter = ospf6_route_next(brouter); | ||
| } | ||
|
|
There was a problem hiding this comment.
This would be simpler:
nbrouter = ospf6_route_next(brouter);
if (brouter->lock == 1)
continue;
And I'm not sure but maybe we could check the OSPF6_ROUTE_WAS_REMOVED flag instead of the refcount lock.
There was a problem hiding this comment.
Negative, your code is a use after free. brouter is deleted when the refcount drops to zero, which happens inside ospf6_route_next as a call to ospf6_route_unlock (ref). I should have been more specific in the PR description.
There was a problem hiding this comment.
Indeed. Your commit message couldn't be better, it's me who did a poor review here.
Thanks for fixing this long-standing issue, will merge now.
Refcount issue
fixes #1135
During the loop we save a pointer to the next route in the table in case
brouter is deleted during the course of the loop iteration. However when
we call ospf6_route_remove this can trigger ospf6_route_remove on other
routes in the table, one of which could be pointed at by said pointer.
Since ospf6_route_next locks the route that it returns, it won't
actually be deleted, instead the refcount will go to 1. In the next loop
iteration, nbrouter becomes brouter, and calling ospf6_route_next on
this one will finally decrement the refcount to 0, resulting in a free,
which causes subsequent reads on brouter to be UAF. Since the route will
have OSPF6_ROUTE_WAS_REMOVED set, provided the memory was not
overwritten before we got there, we'll continue on to the next one so it
is unlikely this will cause a crash in production.
Solution implemented is to check if we've deleted the route and continue
if so.
Signed-off-by: Quentin Young qlyoung@cumulusnetworks.com