[PROPOSAL] policy: Enable to define policy as go plugin

[iwaseyusuke]

Related to the discussion on Slack, this patch enable to define policy as Golang "plugin" (requires Golang 1.8+).

[dbarrosop]

what do you return if you want to reject the prefix?

[iwaseyusuke]

Just returning nil should work as reject.

[dbarrosop]

can you reload it at runtime if there is any change in the policy? How?

[iwaseyusuke]

Sorry for the delay. It’s a troublesome...
When reloading the confirmation, GoBGP will determine the configured values are updated or not before reload API. Then if the configuration file is not changed, even if the plug in is updated, GoBGP can’t detect change of the plug in's implementation.
So if we need to update and reload the plug in, it is required to change the path to plug in or its name.
For example, policy_a.1.0.so to policy_a.1.1.so or appending the updated timestamp or date.

[dbarrosop]

Thanks for the quick answers. I think renaming things and changing the path is fine. Might be worth adding a note to the README so people is aware this is how policies are updated when you are using this method.

[iwaseyusuke]

Yeah, thanks a lot! I will add description about how to update the policy plug-in and how to reject the given prefix.

[dbarrosop]

Don't you have to update this as well? https://github.com/osrg/gobgp/blob/master/api/gobgp.proto#L1076

I tried rebuilding my grpc bindings for python and the following didn't work:

>>> import gobgp_pb2
>>> policy = gobgp_pb2.Policy()
>>> policy.name = "a_policy_that_uses_go_code"
>>> policy.plugin_path = "/path/to/policy.go"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Policy' object has no attribute 'plugin_path'

Or am I missing something?

[iwaseyusuke]

Sorry, I haven't yet consider the implementation for the gRPC API, I did just "by using configuration file" case. I will investigate what are required to support this feature on the gRPC API and update PR.

[iwaseyusuke]

@dbarrosop Thanks for reviewing my patch!
I've just update my patch to add descriptions which you pointed out and support the gRPC API (including gobgp CLI).

[dbarrosop]

thanks to you for the patch. I might be a bit short on time to test the gRPC interface in the next couple of weeks but I will try

[cstockton]

What problem is this pull request trying to solve? I'm a bit concerned about adding the ability to load arbitrary code via an RPC. An attacker that gains access to RPC calls today can only impact BGP peer sessions, which likely have their own peering policies to limit the scope of accepted advertisements. With the attacker could start a remote shell as the GoBGP user and look for potential privilege escalations.

In addition plugins can never be unloaded, further dwindling the benefits of remote management. Plugins also introduce programming errors for policies at runtime as well as allows writing policies that could remove determinism. One run of GoBGP given the same set of policies may not yield the same set of paths. Configuration through the current policy management do not suffer from these same problems.

It's not my place to gauge value of features, I'm not against having plugins. It may solve some interesting use cases for people. I strongly believe it should not be part of the RPC. The plugin path should be a command line flag / env var like "--plugin-path" - that is loaded once at startup. Each plugin could have a name of some type, that is referenced by the policy config instead of a real physical file path. I suggest a "name" because I would like to avoid any file references in the RPC- as then you need to be careful of directory traversals "../../../tmp/somefile.o".

[iwaseyusuke]

@cstockton Hi,
Thanks for your comments! Also sorry for the deley.

First this PR tried to provide the more flexible way to describe policies rather than OpenConfig. Using OpenConfig is good to fit the standard, but I think it is complex to optimize what we want to do with the policies. So I tried to provide the programmable way to do "arbitrary" actions.

Second, this approach is very experimental and as you pointed out, this PR seems to have some security problems.
Passing the plugin path via the commandline arg or var env look good idea for me. Thanks.

[dbarrosop]

I'd like you to consider the attack vector for this particular feature. The grpc endpoint allows you to specify the local path of a plugin that can act as policy. This means for an attacker to do any harm it needs to compromise the server first and be able to upload code prior to change the policy. In summary, you need to compromise two systems; the server and whatever is securing the grpc endpoint.

Also, if an attacker can upload arbitrary code to your server you have bigger problems than their ability to influence your BGP policy and it also invalidates the premise that passing it via local configuration or the command line as everything the attacker needs to do is upload the file and restart the service or wait until an operator does.

[cstockton]

@dbarrosop I believe you are misinformed on multiple fronts based on your response and the lack of security considerations in the code itself. This is completely okay, it's why I'm here having this dialog with you. Lets start by reasoning through your first counter argument, which is essentially this:

In order for a client to set a malicious path, they must compromise the server to upload the file. If they compromise the server they wouldn't need to use the client to set the file path. (I can infer by compromise here, you mean obtain get a user login.)

This argument can be proven false quite simply. The number of potential attack vectors is now the currently undefined total number of mechanisms to put a file on the file system. This is immeasurable risk that will vary greatly across each system configuration. Meaning in order for your feature to be secure you rely on factors that aren't constants you control. An attacker only has to find a service that puts memory they control anywhere on the file system, this is undefined, but lets take some concrete examples:

• An innocent FTP server that has properly jailed uploads to /var/ftp/chroot/ to pull mrt dumps suddenly becomes a vector for attack.
• A well known system logging service XYZ is improperly configured to listen on a public port. It's in an upcoming sprint to fix, but it's not a priority because it's just for telemetry or logging lets say. Well- that program dumps invalid JSON POST requests to a /var/crash/<xyz>/<unixtimestamp>.json folder. An attacker sees it during the port scan, fingerprints it and then simply posts the remote code to execute. It then starts trying to load in a loop for i in 3600; do gobgploadplugin("/var/crash/<xyz>/" + str(<posttime>-N) + ".json"); done - game over.

A motivated attacker may not find a way to get a file on the file system, but that isn't an assumption you may make when writing software responsibly. We have to assume they may find a way and measure the consequence. Compromising of systems is usually done through multiple steps, small escalations in privilege that start with something innocent and build up using assumptions made in this pull request.

I hope the GoBGP authors reconsider being a privilege escalation stepping stone.

Security aside:

• There is no mechanism to unload the plugins, so they will accumulate indefinitely until the server restarts. Forcing the command line to be the control point will naturally prevent accumulation of wasted memory.
• This is compiled code, if each update has to have a file upload to the GoBGP machine, why not just update the config and reload while you're at it? It seems more complex to try to upload it then reload via GRPC.

[dbarrosop]

If your counterarguments are basically (1) a personal attack and (2) designing an inherently insecure system that is a disaster waiting to happen and that would make cry any security engineer consider me out. You clearly don’t have any interest in anything other than shutting this down.

P.S.: if security was really the concern we would be discussing how we can sign and validate the code or other solutions like that

[cstockton]

@dbarrosop First I'm sorry I've made you feel like this was a personal attack, that wasn't my intention. If you could please reach out to me on the GoBGP slack I would be happy to remove any offensive portions of my post. Second I know you put a lot of work into this so I understand how having someone challenge the technical merit of part of it may be discouraging. But I assure you shutting down your pull request as a whole is not my intention here. I would be happy help you shape this into something that is a good compromise between security and your use case.

If you're not interested in working together on this keep in mind I am not a GoBGP maintainer, just a proud user of this fantastic project. So providing some strong counter arguments against my opinions here to help them make an informed decision could be another avenue to take. Maybe you have an example of another large project which allows loading Go plugins via an remote API? We could review issues and pull request to see what kind of engineering rigor went into the feature and how they mitigated security risks.

Finally for your p.s. - I assure you security is my only intention here. I did not provide any methods to mitigate risk because I feel the trade-offs for all of them are inadequate given the critically of the systems GoBGP is used in. I feel the security standards of a networking control plane should have a very high bar. A couple things that may come to mind for you and reasons I'm opposed to them:

Code Signing

I feel the additional complexity added by requirements of secure code signing such as PKI, careful usage of crypto primitives, providing proofs for correctness and potentially adding new dependencies for a feature that few will use is a tough sell. If you did decide to go through the engineering rigor to deliver secure code signing and work through how to make sure GoBGP plugins are disabled by default I would be happy to reconsider my position.

Restrict all plugin files to a specific directory (i.e. http.Dir)

The reason I did not suggest this is because a simple permissions oversight is all that is required to load remote code. It's a nice additional countermeasure but doesn't go far enough.

I think helping me to understand your counter arguments against non-security related arguments may be a good start to move forward:

• There is no mechanism to unload the plugins, so they will accumulate indefinitely until the server restarts. Forcing the command line to be the control point will naturally prevent accumulation of wasted memory.
• This is compiled code, if each update has to have a file upload to the GoBGP machine, why not just update the config and reload while you're at it? It seems more complex to try to upload it then reload via GRPC.