zebra: add PBR script wrapper framework to interact with script #2025
Conversation
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-3209/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base19 Static Analyzer issues remaining.See details at |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
0ecf1bb
to
9411fdd
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-3211/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base19 Static Analyzer issues remaining.See details at |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
9411fdd
to
992eb7f
Compare
|
example of json format returned |
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: FailedFedora24 amd64 build: Successful Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/U1604I386/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI008BLD/config.status/config.status Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI021BUILD/config.status/config.status Warnings Generated during build:Checkout code: Successful with additional warnings:Ubuntu1604 amd64 build: FailedPackage building failed for Ubuntu1604 amd64 build: Ubuntu1604 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI014BUILD/config.status/config.status Ubuntu 16.04 i386: FailedUbuntu 16.04 i386: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/U1604I386/config.status/config.status Debian8 amd64 build: FailedPackage building failed for Debian8 amd64 build: Debian8 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI008BLD/config.status/config.status Debian9 amd64 build: FailedDebian9 amd64 build: config.status output from configure script can be found at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-3221/artifact/CI021BUILD/config.status/config.status |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
zebra/zebra_wrap_script.c
Outdated
| * "opt":"--","in":"any",..}} | ||
| */ | ||
|
|
||
| int zebra_wrap_script(char *script, bool return_data, |
There was a problem hiding this comment.
I know "const" is under used in FRR, but whevenever possible, it should. For instance, here "char *script" is clearly a const.
pguibert6WIND
force-pushed
the
issue_2018
branch
from
992eb7f
to
372764e
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-3225/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base19 Static Analyzer issues remaining.See details at |
|
What's the end goal of this patchset? I see comments in the code to get access to iptables output. Why are we exposing ourselves to allowing the execution of arbitrary scripts? It sure seems if we need access to iptables information from the kernel we should use the kernels abi for it. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
There was a problem hiding this comment.
I have various problems with this code.
- Code usage is unclear and functions are named poorly.
- This is clearly intended to be used for parsing the output of
iptables.iptablesrequires root and therefore you either plan to run zebra as root without privilege dropping, or withiptablesas setuid root. Both of these seem like bad ideas. I also do not understand why we would want to readiptablesrules in Zebra. - The ability to invoke the shell from Zebra makes me uncomfortable and I don't see a strong enough justification for the very likely security holes this will open in the future.
- Any script output not containing a whitespace will segfault the program.
- This code as written contains an exploitable buffer overflow due to an unchecked memcpy. Output exceeding 200 characters followed by a space will overwrite the stack frame of
handle_field_linewith the data between the 201st character and the space.
zebra/zebra_wrap_script.c
Outdated
| } | ||
| } | ||
| if (pclose(fp)) | ||
| zlog_err("NETLINK: error closing stream with %s", script); |
There was a problem hiding this comment.
What does netlink have to do with this code?
zebra/zebra_wrap_script.c
Outdated
| static int zebra_wrap_debug; | ||
|
|
||
|
|
||
| void zebra_wrap_init(void) |
There was a problem hiding this comment.
Unnecessary, statics with automatic storage duration are initialized to zero automatically
zebra/zebra_wrap_script.c
Outdated
| zebra_wrap_debug = 0; | ||
| } | ||
|
|
||
| static int search_current_word(char *current_str, int init, |
There was a problem hiding this comment.
This is clearly built to process iptables output, it should be named as such.
However, this also does essentially the exact same thing as strsep, doesn't it? Have you considered using strsep?
There was a problem hiding this comment.
not yet. I think strsep or strchr could be helpful.
for iptable output process, I think of keeping the name very generic, so that this kind of function can be applied to not only iptables but also other functions ( ipset ?)
zebra/zebra_wrap_script.c
Outdated
| return -1; | ||
| } | ||
|
|
||
| static int handle_field_line(struct json_object *json_obj, |
There was a problem hiding this comment.
Same comment as search_current_word.
| * # iptables -t mangle -L PREROUTING -v | ||
| * Chain PREROUTING (policy ACCEPT 150k packets, 7426 bytes) (## line 0) | ||
| * pkts bytes target prot opt in out source destination (## line 1) | ||
| * 0 0 DROP all -- any any anywhere anywhere |
There was a problem hiding this comment.
This example, and the code above to parse this output, makes me uncomfortable. It implies you are running Zebra as root, or expect users of this to be running Zebra as root, or perhaps that you have setuid root on iptables, all three of which are bad ideas...
There was a problem hiding this comment.
you are right. there are assumptions done.
| if (line_nb > begin_at_line) | ||
| json_obj = json_object_new_object(); | ||
| else | ||
| json_obj = NULL; |
There was a problem hiding this comment.
The only time this executes is when line_nb == begin_at_line, is that the intent?
There was a problem hiding this comment.
idealle, you can split function in two:
- one that looks for headers
- the other one that looks for entries
|
some responses that may help to understand the feature:
I think this pull request ( and the ticket attached) is the good location to discuss on the ability to invoke the shell from Zebra. Also, an other advantage of relying on scripts is in the multiple OS part. Because ABI is clearly identified on Linux, this may not be the case for other Oses. For instance, in order to benefit from iptables on non Linux system, one would have to implement a new backend that would be different from the netlink. Here, if iptables is available on a non linux system, I offer the possibility to have one script backend that is available everywhere. Q2 : why calling from Zebra ? This is contextual. In my case, I make policy-routing from BGP Flowspec.
from the POC perspective, could not we allow ( I don't know, temporarily or on a compilation define basis) the opening to rely on a script extension ? |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
372764e
to
d95940a
Compare
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-3230/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base19 Static Analyzer issues remaining.See details at |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
d95940a
to
457f1bd
Compare
|
The script will evolve, and I will inform you about an update of that script. For the reason why using scripts, I would like some feedback : having a way to rely on scripts instead of netlink could extend the usage of FRR on not only linux platforms. |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
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-3260/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base20 Static Analyzer issues remaining.See details at |
I wonder why you want to upstream this functionality. I understand these wrappers can be useful during the early development phases of a new feature, but at some point you'll need to convert your code to use the right APIs. You can't escape from that, I'm pretty sure no one will merge code that depends on the text output of any script or tool. You might argue these wrappers could benefit other developers, but I'm skeptical about that. Wouldn't it be possible for you to use these wrappers internally only?
The text output of several tools varies a lot among different platforms, relying on any specific format is just a bad thing to do. |
|
In general I agree with @rwestphal that this is of dubious value for anyone. Especially since you'll be doing work twice( once in a script and once implementing the proper api ). Having said that, I do believe if the cli and the functionality here are wrappered inside of a module and a configure compile option the chance for damage is low and the code would either be maintained by @pguibert6WIND or not. If not maintained it would stop working and that is no skin off of my back. |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
457f1bd
to
ab13bd1
Compare
pguibert6WIND
changed the title
|
@qlyoung , I will look into your comments, since I refactored the whole code. because the parsed string may not have only one space, strchr would stop at the first occurency of space, while there are other occurences of spaces. For your remarks, I think I will still need to review with your help.
About the wrap script I done, let things be clear:
for comments from @rwestphal and @donaldsharp , I think there has been acceptance of a using a module to host those wrap script apis. does not that mean that it is more than half-accepted ? |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
|
Answering myself:
Watch flows, call scripts based on policy and keep statistics. Is this correct?
This is still unanswered. I'd expect at least a new output format from your scripts (or input) so we don't break on external tools change (
This is also unanswered and it has been questioned by others. It is clear that implementing this feature here will severely degrade security and our capacity to lock down (at least a. you are proposing to add more privileges to zebra, when it only need a few to open some system sockets; Most of the points above can be avoided by using the kernel APIs directly. If you still want to go this way, why is this being implemented in |
|
This paragraph displays a major misunderstanding of security design and impact:
An administrator has a reasonable belief that if a major software application provides a feature, that it is reasonably secure, and will remain that way. That's not the case here. The slide deck gives no indication that other designs or methods of implementation were considered, and why they weren't adopted. Have you involved other folks at your company, particularly those concerned with software security? If not, I think that would be a really good idea. I'm still seeing no reason from the slide deck that this would be best implemented directly inside zebra or other frr components via running programs and reading their output. |
|
about the script configuration, the plugin module provides some script: also, I plan to update the documentation. here is the first extract I hope the above answers point 2 from Rafael. For point 3, security, I say that zebra may be open a secuty hole, but I am sure the administrator has a full set of tools to manage not only FRR but the environment around. There are a lot of linux tools that permit customize security, by taking into account not only FRR but also some external scripting that may be called. For point 3, netlink. iptables relies on ioctl, not netlink. |
|
ok. For security concerns, it seems everybody is against that ticket. However, the script answers my needs as I depicted it in the slide. |
In your analogy, my opinion is that every brick ought to be as protected as it reasonably can be. Just because there are other methods of shielding FRR from attack does not mean we can ignore security from the FRR perspective. One might just as easily say "we have firewalls, so we can trust everything we receive on the network." |
|
To @rzalamena ,
the purpose is both configuring Netfilter through scripting., and keep statistics.
I can configure the call in the script.
I admit those are unanswered issues.
maybe Rafael, you could help me on that point. |
pguibert6WIND
force-pushed
the
issue_2018
branch
from
ad4c313
to
17e3736
Compare
Add ns_id into zebra_pbr ipset This is important so that each ipset entry knows on which NETNS the ipset entry must be inkected Signed-off-by: Philippe Guibert <[email protected]>
This commit is a fix that removes the structure from the hash list, instead of just removing that structure. Signed-off-by: Philippe Guibert <[email protected]>
Upon the remote daemon leaving, some contexts may have to be flushed. This commit does the change. IPset and IPSet Entries and iptables are flushed. Signed-off-by: Philippe Guibert <[email protected]>
In cast the removal of an iptable or an ipset pbr context is done, then a notification is sent back to the relevant daemon that sent the message. Signed-off-by: Philippe Guibert <[email protected]>
When a mark is set, incoming traffic having that mark set can be redirected to a specific table identifier. This work is done through netlink. Signed-off-by: Philippe Guibert <[email protected]>
This framework has 2 APIs, - able to execute script show command, and return the output in a json structure. Those script show command are tighted with iptables and ipset output. - able to analyse json tree, and extract pkts and bytes values. This framework permits gaining time, since it allows frr to call some external programs and rely on externals like the output. This framework relies on plugin module. This module is called zebra_wrap_script. Signed-off-by: Philippe Guibert <[email protected]>
update documentation with zebra module script. Signed-off-by: Philippe Guibert <[email protected]>
In order to configure iptables, ipset entries and ipset contexts, 2 script handlers apis are available to pass scripts commands. Signed-off-by: Philippe Guibert <[email protected]>
This preparatory work introduces a new node that will be used to add vty configuration commands, and the associated show running. Signed-off-by: Philippe Guibert <[email protected]>
3 new vty commands permit to configure zebra wrap ip scripts: - [no] wrap script ipset <> - [no] wrap script iptable <> - [no] wrap script iprule <> Signed-off-by: Philippe Guibert <[email protected]>
The following PBR handlers: ip rule, ipset, and iptables will prioritary call the wrap script handlers. If the script handler is not present ( or returns 0 - that is why the script handlers may not return 0), then if available an other configuration call may be called. This is the case with ip rule PBR handler that also has a netlink API. This mechanism guarantees that if wrap script can configure it, then no need to use netlink. Signed-off-by: Philippe Guibert <[email protected]>
Two new vty show functions available: show pbr ipset <NAME> show pbr iptables <NAME> Those function dump the underlying "kernel" contexts. It relies on the zebra pbr contexts first. This helps then to know which zebra pbr context has been configured since those contexts are mainly configured by BGP Flowspec. Also, it relies on zebra wrap context API. From this wrap API, it gets some statistics information to know which context has been matching how many packets and bytes. Signed-off-by: Philippe Guibert <[email protected]>
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-3486/ This is a comment from an EXPERIMENTAL automated CI system. Warnings Generated during build:Checkout code: Successful with additional warnings:CLANG Static Analyzer Summary
No Changes in Static Analysis warnings compared to base6 Static Analyzer issues remaining.See details at |
💚 Basic BGPD CI results: SUCCESS, 0 tests failedResults table
For details, please contact louberger |
This is the wrong attitude to have. It is not acceptable to knowingly introduce security issues in software simply because external mitigations / permission systems exist. If e.g. Apache were to take this stance and respond to security issues with something like "Well, you should just run Apache in an isolated VM with strict SELinux policies" then Apache would die a swift and irreversible death. In short, from a security standpoint, I expect every program in this suite to be as bulletproof and idiot-proof as we can possibly make them and this PR runs totally contrary to that objective. See also: While they do claim that "it is recommended to [...] use system()" IMO this is not an acceptable answer in FRR. As naïve as it is to suggest, your efforts might be best spent on contributing an API to netfilter / iptables rather than parsing fragile text output here. |
When a BGP flowspec peering stops, the BGP RIB entries for IPv6 flowspec entries are removed, but not the ZEBRA RIB IPv6 entries. Actually, when calling bgp_zebra_withdraw() function call, only the AFI_IP parameter is passed to the bgp_pbr_update_entry() function in charge of the Flowspec add/delete in zebra. Fix this by passing the AFI parameter to the bgp_zebra_withdraw() function. Note that using topotest does not show up the problem as the flowspec driver code is not present and was refused. Without that, routes are not installed, and can not be uninstalled. Fixes: 529efa2 ("bgpd: allow flowspec entries to be announced to zebra") Link: FRRouting#2025 Signed-off-by: Philippe Guibert <[email protected]>
When a BGP flowspec peering stops, the BGP RIB entries for IPv6 flowspec entries are removed, but not the ZEBRA RIB IPv6 entries. Actually, when calling bgp_zebra_withdraw() function call, only the AFI_IP parameter is passed to the bgp_pbr_update_entry() function in charge of the Flowspec add/delete in zebra. Fix this by passing the AFI parameter to the bgp_zebra_withdraw() function. Note that using topotest does not show up the problem as the flowspec driver code is not present and was refused. Without that, routes are not installed, and can not be uninstalled. Fixes: 529efa2 ("bgpd: allow flowspec entries to be announced to zebra") Link: FRRouting#2025 Signed-off-by: Philippe Guibert <[email protected]>
When a BGP flowspec peering stops, the BGP RIB entries for IPv6 flowspec entries are removed, but not the ZEBRA RIB IPv6 entries. Actually, when calling bgp_zebra_withdraw() function call, only the AFI_IP parameter is passed to the bgp_pbr_update_entry() function in charge of the Flowspec add/delete in zebra. Fix this by passing the AFI parameter to the bgp_zebra_withdraw() function. Note that using topotest does not show up the problem as the flowspec driver code is not present and was refused. Without that, routes are not installed, and can not be uninstalled. Fixes: 529efa2 ("bgpd: allow flowspec entries to be announced to zebra") Link: FRRouting#2025 Signed-off-by: Philippe Guibert <[email protected]>
When a BGP flowspec peering stops, the BGP RIB entries for IPv6 flowspec entries are removed, but not the ZEBRA RIB IPv6 entries. Actually, when calling bgp_zebra_withdraw() function call, only the AFI_IP parameter is passed to the bgp_pbr_update_entry() function in charge of the Flowspec add/delete in zebra. Fix this by passing the AFI parameter to the bgp_zebra_withdraw() function. Note that using topotest does not show up the problem as the flowspec driver code is not present and was refused. Without that, routes are not installed, and can not be uninstalled. Fixes: 529efa2 ("bgpd: allow flowspec entries to be announced to zebra") Link: FRRouting#2025 Signed-off-by: Philippe Guibert <[email protected]>
This framework has 2 kinds of APIs:
This framework permits gaining time, since it allows frr to call some
external programs and rely on externals like the output.
Signed-off-by: Philippe Guibert [email protected]