Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"--ppd-plugin" and "--ppd-log" parameters may be a security risk when running through sudo #54

Closed
mata-p opened this issue Apr 15, 2016 · 2 comments

Comments

@mata-p
Copy link

mata-p commented Apr 15, 2016

When execution of the openfortivpn executable is granted to unprivileged users the way the example on the project frontpage shows, the --ppp-plugin and --ppd-log arguments are passed directly to pppd as plugin, respective logfile.

As pppd in this case will be executed as root, plugin allows to load a mailicious user controlled plugin from an arbitrary location as root. This could probably be mitigated by only allowing plugin names which don't contain a slash, therfore limiting loading plugins only from the /usr/lib/pppd/[version] directory.

logfile allows to open (or create) an aribitray file as root and append log output to it. pppd when not invoked as root opens the log file as the original user, but in this case it's already invoked as root thorugh sudo.

At the very least the instructions on the front page should be changed not to allow direct execution of the openfortivpn binary but of a wrapper script that doesn't take potentially insecure user arguments.

@mata-p mata-p changed the title "--ppd-plugin" and "--ppd-log" parameters may be a security risk when running as root "--ppd-plugin" and "--ppd-log" parameters may be a security risk when running through sudo Apr 15, 2016
adrienverge added a commit that referenced this issue May 21, 2016
@adrienverge
Copy link
Owner

@mata-p,

Thanks and sorry for the long delay.

I've just added a section to briefly warn about the problem in the README file. I'm open to any better solution ; pull requests are welcome.

@mrbaseman
Copy link
Collaborator

I think we can close this, since the readme has been updated a long time ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants