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

System Wizard performs variable interpolation on PPPoE password #1359

Closed
xtof01 opened this Issue Jan 28, 2017 · 20 comments

Comments

Projects
None yet
5 participants
@xtof01

xtof01 commented Jan 28, 2017

Hi,

I noticed a potentially very annoying behaviour of the System Wizard. When configuring the WAN interface for PPPoE, a password that contains a dollar sign character seems to be subjected to variable interpolation before getting stored in the configuration.
If, for example, I enter

ab$h7v(ax

as PPPoE password, what gets actually stored in the configuration is the string

ab(ax

i.e. the $h7v part gets stripped.

This is very easily reproducible:

  1. Install opnsense in VM or use existing installation
  2. On the web interface, go to System, Wizard
  3. Click Next three times
  4. Set Selected Type to PPPoE
  5. enter any PPPoE username
  6. enter ab$h7v(ax as PPPoE password
  7. Click Next three times, then Reload
  8. Go to System, Configuration, Backup
  9. Download the configuration
  10. Open the file with a text editor, find the password in the <ppps> section.
  11. The base64-encoded password is YWIoYXg=.
  12. base64-decoding it yields ab(ax.

This bug applies to both 16.7 and 17.1 (Beta and RC1).

The reason why I wrote above that this is potentially very annoying is that some providers (e.g. Deutsche Telekom) will block your account for up to 24 hours after a few login attempts with an invalid password ... 😞

@fichtner

This comment has been minimized.

Member

fichtner commented Jan 28, 2017

Hi,

This is quite terrible from a security perspective. The bug seems to be present in our parent project as well. Here's a patch to run from the command line:

# opnsense-patch 593d7525

Unfortunately, this is too late for 17.1 to fix, the bug will be in the final images as it always was before.

Can you let me know if that works?

Thank you,
Franco

@fichtner fichtner added this to the 17.7 milestone Jan 28, 2017

@fichtner fichtner added the bug label Jan 28, 2017

@fichtner fichtner self-assigned this Jan 28, 2017

@xtof01

This comment has been minimized.

xtof01 commented Jan 29, 2017

Hi,

that was fast 😃
Yes, I can confirm that the patch works!

Thanks,
Christof

@fabianfrz

This comment has been minimized.

Member

fabianfrz commented Jan 29, 2017

@fichtner Should a CVE number or something like that be assigned?

@fichtner

This comment has been minimized.

Member

fichtner commented Jan 29, 2017

@fabianfrz It's strictly an unwanted information disclosure, but only from a privileged account as it needs admin rights to run the wizard, which also changes the root password. I don't think this is worth it, just very bad style because of the bug that was reported here that shouldn't have happened in the first place.

FWIW, the old wizard framework should be removed in its entirety and upgraded in features. This is the final straw and we'll put this on the 17.7 roadmap.

@xtof01 thanks for confirming :)

@fichtner fichtner closed this Jan 29, 2017

fichtner added a commit that referenced this issue Jan 29, 2017

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

@xtof01 can you drop me a private mail? franco AT opnsense DOT org

@gonzopancho

This comment has been minimized.

gonzopancho commented Feb 9, 2017

Concerning the statement, "It's strictly an unwanted information disclosure,"

It's an RCE, not information disclosure, so much more dangerous than Franco let on.

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

@gonzopancho in the scope of what we have discussed, no. we only talked about "$" evaluation, not code injection leading to RCE. Still:

The impact is questionable as the attack requires a valid CSRF token and a wizard privilege, which, incidentally, allows users to run the general setup wizard including changing the admin/root password and by default the admin is the only one who can grant it. That's why we pursued it no further.

And besides, we fixed the RCE without even knowing about it. :)

In any case, thank you for the coordination.

Cheers,
Franco

@fabianfrz

This comment has been minimized.

Member

fabianfrz commented Feb 9, 2017

@fichtner why we? That is why I have asked about a CVE number ;)

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

@fabianfrz if you want a CVE number, be my guest to request it. The bottom line still stands: it needs admin/root rights to trigger it in a default install.

@gonzopancho

This comment has been minimized.

gonzopancho commented Feb 9, 2017

As Tim Coen wrote in his original assessment, it can be used to execute commands as well, it isn't only information disclosure. Your fix still passes user input to eval. Using single quotes and addslashes
may be sufficient, for now. If the future finds a different hole, you'll have to remember to fix this, again. This is why we prefer the fix in pfsense/pfsense@5baea4d
Your commentary there would be comedy gold, were it not so antagonistic. Your ego is not your amigo.

To address your comments directly above: While the barrier to entry is high, and thus the actual impact is minimal, this is still a potential problem that needs disclosure.

I think you're either confused or attempting to misdirect with "requires a valid CSRF token".

The real issue is someone gaining elevated privileges and/or setting up a backdoor.

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

Tim agreed that the use of single quotes is sufficient. Your sentiment wether the fix is good enough in your view or not is irrelevant. And if you think that ad hominem attacks help your case, so be it.

We had no notice, found the problem on our own before it even was reported to you. We fixed it correctly. We shipped it today. Problem solved. :)

Cheers,
Franco

@jschellevis

This comment has been minimized.

Member

jschellevis commented Feb 9, 2017

Well done Franco! Thank you for all your efforts to keep OPNsense secure and safe.

Cheers,

Jos

@gonzopancho

This comment has been minimized.

gonzopancho commented Feb 9, 2017

"found the problem on our own" ... no you didn't, someone opened that issue and reported it to you.

Then you neglected to report it upstream.

Someone else said this today, "the dogs bark, but the caravan moves on."

https://www.mail-archive.com/freebsd-ports@freebsd.org/msg72696.html

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

You are not upstream. :)

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 9, 2017

PS: ad hominem, strike 2

@gonzopancho

This comment has been minimized.

gonzopancho commented Feb 10, 2017

You are not upstream.

Then I'll use your words: "parent project". #1359 (comment) The point still stands. You neglected to report to pfSense even though, as you stated above, you knew that the issue existed.

You subsequently stated, "... if this is a security issue I ask to share the details with us, because we are also affected." pfsense/pfsense@5baea4d#commitcomment-20815871

This type of behavior is, by any measure, duplicitous. That's not "ad hominem", either, since it's a statement of fact.

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 10, 2017

Dear @gonzopancho,

I shared info with @jim-p in the past. And I cannot remember a time when I didn't have to ask to receive the likewise. I got the particular details when I asked about his bugfix, which aligned 100% in spirit with a commit that we did just over a week before. The timing was coincidental after all, but if you want to be angry for me asking about it and assert that your fix is better when in fact (a) we never had the researcher information you had and (b) the code is safe as confirmed by the researcher himself. You have to realise the time line does not fit your narrative.

Let me spell out your general position:

OPNsense only gets advisories when pfSense releases them to the public. We are okay with it, we have to. But don't come asking here for more than you are willing to give.

And, again, the scope of this security issue:

This flaw requires the unique wizard privileges, which gives the user control over the general setup including resetting the root password. This is only given to admins who have full control. And only admins can set it for normal users. When you talk about privilege escalation, you don't need this bug to compromise a system, because you already have a compromised system by gaining these wizard privileges. Thus I cannot see a real world application that doesn't involve another serious bug we should be focusing on instead.

Let me go one step further:

OPNsense is 2 years old and a user reported this particular issue. The problem was immediately found and fixed. pfSense has the code much longer and according to you many more users, so I find it odd that the bug wasn't reported by one of your users in the past and/or fixed. It even took a researcher to point it out to you.

Do you want to discuss this further? Or will you agree that both projects did a fine job in fixing it and we can all move on with doing good work? Or do you want to change the way we work together by starting to work together?

I'm happy for a change. :)

Cheers,
Franco

@gonzopancho

This comment has been minimized.

gonzopancho commented Feb 10, 2017

Franco,

But don't come asking here for more than you are willing to give.

To be clear, I wasn't asking you for anything.

I started out welcoming OPNsense (https://forum.pfsense.org/index.php?topic=86170.0), and have attempted to extend a hand in the past.

You stated that you wouldn't take a simple pull request for inserting a space between two words in your README.md, and had someone else duplicate the (trivial) work.

5489e8e

There are other, more incendiary responses from you, but these would require that I expose email from you.

If you're saying you've changed your heart now, let me know.

Jim

@jschellevis

This comment has been minimized.

Member

jschellevis commented Feb 10, 2017

Jim, you have been spreading a lot of nonsense and you do not seem to grasp the concept of open source.

Please leave and work on your own project, you have a lots of promises that still need to be delivered upon.

@fichtner

This comment has been minimized.

Member

fichtner commented Feb 11, 2017

Jim,

Since you have nothing to add to my assessment of the scope of this security issue, I take it that you agree with it or at least have no more base for argument. I've also added a follow-up commit to split up the privileges for the wizard to avoid giving an OpenVPN wizard user the ability to run the general setup wizard. Feel free to pick it up and consider pfSense "notified".

Since you have nothing to add to me saying I've reported issues to you, I would just like to note that when I point out issues in pfSense, I'm met with criticism and/or being ignored. This is a self-made issue you need to address:

pfsense/pfsense@33f0b0d#commitcomment-14216708

Funnily enough, the particular issue was fixed after labeling my thoughts on the matter "opinion". When you later act on those opinion you oppose, you are going into hypocrisy territory. We've had this discussion before. It keeps happening on a regular basis, but how you want to run your project is your thing.

On contribution I must say you changed pfSense to be a 6-Clause ESF License and added a CLA which gave you full rights of every contribution and we said no thank you. OPNsense was based on retaining 2-Clause BSD and no CLA. You could have had all the OPNsense code. pfSense would have been a killer project. You created this situation by trying to assert full control over an open source code base, so please live up to the reality you created. You can still have all the code and win big time. But you are not.

I'm considering this closed now.

Cheers,
Franco

@opnsense opnsense locked and limited conversation to collaborators Feb 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.