latest stable versions: v150827 (changelog)

Old Forums (READ-ONLY): The community now lives at WP Sharks™. If you have an s2Member® Pro question, please use our new Support System.

Possibly SQL Injection issue

Home Forums Community Forum Possibly SQL Injection issue

Tagged: 

This topic contains 20 replies, has 4 voices. Last updated by  Jason (Lead Developer) 4 years ago.

Topic Author Topic
Posted: Friday Dec 28th, 2012 at 12:04 pm #35626
Scott Paley
Username: spaley

Hello,

We are about to launch a new website that uses s2member. The site will be featured on a major network TV show in 9 days. The network asked us to run the site code through Veracode for security purposes and we found a bunch of issues in s2member, including SQL injections risks.

I then set up individual scans for s2member and s2member-pro. I’m still waiting for the results from the former but I already have the latter. In “pro” there were 2 SQL injection issues that came up.

I figured you’d want to know about this ASAP as this could present a major problem for you (and for us until this is resolved.) I’m happy to share the “pro” results report now and the s2member results once they complete. I don’t want to share them with everybody on this forum though so please let me know the best way to get this to you.

Scott

List Of Topic Replies

Viewing 20 replies - 1 through 20 (of 20 total)
Author Replies
Author Replies
Posted: Friday Dec 28th, 2012 at 12:19 pm #35630
Scott Paley
Username: spaley

The second report just came in and reported 196 possible code injection flaws, 4 XSS flaws, and several other smaller issues.

Again, please let me know how I can provide you with the specific information in a private way.

Posted: Friday Dec 28th, 2012 at 12:20 pm #35632
David Welch
Username: dwbiz05

Does the report show an actual sql injection they tried and what was returned?

Just wondering what they consider a “flaw” and how they come up with that.

Also, did you make sure that s2member was the only thing functioning? No special themes or plugins?

Dave

  • This reply was modified 4 years ago by  David Welch.
Posted: Friday Dec 28th, 2012 at 12:22 pm #35634
Scott Paley
Username: spaley

David – I’m not certain. It’s a static scan, not done manually. It’s entirely possible it’s found some “false positives.”

That said, this is certainly an urgent concern for us and I’d imagine for s2member as well.

Posted: Friday Dec 28th, 2012 at 12:23 pm #35635
Scott Paley
Username: spaley

On your last question about shutting off other plugins, for these 2 reports I ONLY uploaded the 2 plug-in folders (s2member and s2member-pro). There are no other plugins, or even WordPress itself, involved here.

Posted: Friday Dec 28th, 2012 at 12:27 pm #35636
David Welch
Username: dwbiz05

See that’s my concern with a report like this. If they just look through your source files and don’t actually test the site, then they could be making mistakes. See, s2member typically uses wordpress database interaction which (to my understanding) is already protected. However, if this scanning type doesn’t recognize that, then it would flag it.

I would be interested in seeing some examples… hopefully Jason or someone will see this and get the info and let us know the status.

Thanks,

Dave

Posted: Friday Dec 28th, 2012 at 12:28 pm #35637
Scott Paley
Username: spaley

Yes, I agree with you. I’m hoping it’s all just false positives. But… the network won’t sign off on the website until we can either prove it’s a false positive, or if it’s not, that it gets fixed.

And if it really IS an issue, it’s a pretty major issue that will affect all s2member websites and really should be fixed urgently.

Posted: Friday Dec 28th, 2012 at 12:30 pm #35638
Scott Paley
Username: spaley

BTW – at first glance, one of my developers is telling me that these look like real issues. He wrote, “I haven’t tried to actually exploit them, so I’m not 100% positive, but it definitely looks like user input that’s not being sanitized.”

Posted: Friday Dec 28th, 2012 at 12:31 pm #35639
David Welch
Username: dwbiz05

You might try contacting them with this info via this contact form as well:

https://www.s2member.com/contact/?s2-ssl=yes

Dave

Posted: Friday Dec 28th, 2012 at 12:35 pm #35641
Scott Paley
Username: spaley

Thanks – will try that.

Posted: Friday Dec 28th, 2012 at 12:37 pm #35642
Scott Paley
Username: spaley

Adding “security” as a tag.

Posted: Friday Dec 28th, 2012 at 1:34 pm #35647
Staff Member

Hi Scott. Thanks for reporting this important issue.

Please submit copies of your reports to us via this private contact form. I recieved a form submission from you, but I don’t have copies of the reports that you received yet. See: s2Member® » Private Contact Form

We take security very seriously here. I’m not aware of any XSS or SQL injection risks with s2Member or s2Member Pro, but we’ll be happy to review reports produced by your scanner with you. If you would prefer to post them publicly here in the forum, that’s fine also. s2Member’s source code is open.

Posted: Friday Dec 28th, 2012 at 2:18 pm #35653
Scott Paley
Username: spaley

Thanks Jason,

I just sent the reports. I’m sure you take security very seriously, which is why I brought this to your attention immediately. I’d be thrilled if you could prove to me these are all false positives, but if not, we really need these fixed urgently (as I’m sure do many of your users.)

Posted: Friday Dec 28th, 2012 at 2:24 pm #35654
Scott Paley
Username: spaley

FYI, it’s possible we’ll have time to do a manual scan as well over the weekend. We are having Veracode run a dynamic scan to get additional results, but it may take a few days to get the report on that.

Posted: Friday Dec 28th, 2012 at 2:39 pm #35656
Scott Paley
Username: spaley

Just to be clear about the scan. We uploaded all of our source code to Veracode, which has some method of analyzing all the code and finding security holes. The output of that was what raised our alarms.

The dynamic scan we’ll run over the weekend is more like Selenium. It’ll actually mimic a user trying to break the site from the front-end. I’ll be happy to share those results as well.

And of course, the manual scan is literally a human being trying to hack the site via methods like SQL injection, XSS, or other nefarious means.

Posted: Monday Dec 31st, 2012 at 1:31 pm #35857
Staff Member

Reports received. Thank you.

I’ve just finished a careful review of the reports that you submitted privately. I’ll post my findings below for you. In short, I did not find any legitimate concern in this scan. Yes, please do let us know if your additional scans turn up any problems on your installation. We’ll be happy to review those with you as well.
Analysis of security scan

1. Improper Neutralization of Directives in Dynamically Evaluated Code (‘Eval Injection’) (CWE ID 95)(194 flaws)
– Also found in s2Member Pro: Improper Neutralization of Directives in Dynamically Evaluated Code (‘Eval Injection’) (CWE ID 95)(2 flaws)

These are all false positives. It’s not uncommon for a scanner to pick up on these instances of the eval() PHP function, simply because the use of the eval() function in PHP, is prone to security issues when not used properly. s2Member’s use of the PHP eval() function is secured against untrusted PHP code; and thus is NOT a security issue in s2Member. Upon closer inspection (one which is NOT automated), one will find that s2Member uses the eval() function to make s2Member more accessible for plugin developers, and also to evaluate PHP code provided by advanced site owners through UI panels in the Dashboard, made available ONLY to Administrators of the site. s2Member does not make it possible for code injections be to evaluated via the PHP eval() function, or from any source other than a site Administrator.

ADDITIONAL NOTES: The use of the eval() function in PHP, is prone to security issues when not used properly. However, there are justifiable reasons to use the PHP eval() function, and s2Member’s use of eval() is justified in our opinion. Please pass this along to your security review team.

2. Improper Control of Filename for Include/Require Statement in PHP Program (‘PHP File Inclusion’) (CWE ID 98)(2 flaws)

These are both false positives. The automated scanner has simply picked up on variables being used in an include/require call, but has not considered whether those variables came from trusted or untrusted sources. In both of these “flaws” that your scanner picked up, the variables can most definitely be trusted. They do not come from untrusted sources in any way. In fact, they are absolutely required for s2Member to function properly. These variables are established at runtime by the s2Member application, using trusted data sources.

3. Improper Neutralization of Script-Related HTML Tags in a Web Page (Basic XSS) (CWE ID 80)(4 flaws)

All four of these were false positives. I have no explanation for why your scanner would have picked up these. It appears these were simply chosen by heuristics which might be incapable of detecting true security issues. I found nothing in any of these cases that was questionable.

4. Use of a Broken or Risky Cryptographic Algorithm (CWE ID 327)(77 flaws)
– Also found in s2Member Pro: Use of a Broken or Risky Cryptographic Algorithm (CWE ID 327)(33 flaws)

These are all false positives. Your scanner has simply picked up on s2Member’s use of the MD5() function in PHP, which has MANY justifiable uses throughout s2Member’s source code. While the MD5() hashing algorithm has been found to have some weaknesses when compared to more modern hashing algorithms, s2Member’s use of the MD5() function in PHP is not targeted to any sensitive data, rather it is simply building a hash for low-level data comparision. Anytime s2Member is truly encrypting data, that encryption is performed by s2Member using MCRYPT_RIJNDAEL_256, which a high-level encryption technique. Much more secure than a simple call to MD5().

NOTE: You will find many valid uses for the MD5() function in PHP; throughout many plugins/themes. Quite common.

5. External Control of File Name or Path (CWE ID 73)(1 flaw)
– Also found in s2Member Pro: External Control of File Name or Path (CWE ID 73)(10 flaws)

These are false positives. The arguments are passed by s2Member internally, and are not received from any unknown or untrusted source. This looks more like a scanning error to me. I’m really not sure why a scanner would ever be this senstitive. Or if it was going to be this sensitive, it should really backtrace where data is coming from, instead of assuming that data is untrusted. In any case, I suppose a manual review is better than no review at all. My impression of this scanner is that it’s assuming the worst, and asking you or your developer to confirm validity in the code. These are false positives.

6. Improper Neutralization of Special Elements used in an SQL Command (‘SQL Injection’) (CWE ID 89)(3 flaws)
– Referencing: trunk/…/utils-users.inc.php 42

This is a false positive. The variables being referenced are supplied by the WordPress database class itself, and have already been sanitized. In fact, the variables in question are actually referencing table names, as defined by the WordPress database class (ex: $wpdb->users, $wpdb->usermeta, etc). False positive.

Please let us know if we missed anything. Thanks!

Posted: Monday Dec 31st, 2012 at 1:46 pm #35859
Scott Paley
Username: spaley

Jason – thank you so much for your thoroughness. I’ll be passing your findings on to our security team. I have not yet gotten the results of the dynamic scan, but if anything turns up that seems due to s2member, I’ll let you know.

Really appreciated and happy new year!

Posted: Wednesday Jan 2nd, 2013 at 7:15 am #35964
Scott Paley
Username: spaley

Jason –

The s2member-pro Veracode report shows 2 code injection issues at the file s2member-pro\includes\classes\login-widget.inc.php.

It uses $_SERVER[“REQUEST_URI”] in raw form to echo it out in HTML (into an field and into a link).

The value of $_SERVER variables can’t be trusted, as it can be tampered by the end user.

This is why the WP core uses the esc_url() function whenever WP needs to use REQUEST_URI.
You’re using esc_attr instead, which doesn’t seem to provide enough URL cleaning, and thus open this input for XSS.

Could you comment on this?

Posted: Friday Jan 4th, 2013 at 3:19 pm #36185
Scott Paley
Username: spaley

Can I please get an update on this query?

Posted: Saturday Jan 5th, 2013 at 2:29 pm #36303
Eduan
Username: Eduan
Moderator

Hello Scott,

I contacted Jason reminding him of this post. He probably missed the email notifications. :)

– Eduan

Posted: Saturday Jan 5th, 2013 at 8:35 pm #36362
Staff Member

Thanks for the follow-up.

It uses $_SERVER[“REQUEST_URI”] in raw form to echo it out in HTML (into an field and into a link).

The value of $_SERVER variables can’t be trusted, as it can be tampered by the end user.

This is why the WP core uses the esc_url() function whenever WP needs to use REQUEST_URI.
You’re using esc_attr instead, which doesn’t seem to provide enough URL cleaning, and thus open this input for XSS

Just to clarify… s2Member is not using this variable $_SERVER["REQUEST_URI"] in raw form for display, we’re wrapping it with esc_attr(), which sanitizes this variable for display in any HTML attribute, by removing these potentially harmful characters. Thus, preventing the possibility of XSS where this data is concerned.

I understand your desire to follow WordPress standards to the letter on this, by using the specific esc_url() function. However, I’m not aware of any vulnerability arising from our use of esc_attr().

This is a multipurpose function that will sanitize ANY HTML attribute, regardless of what one might consider the value to be going into a sanitation routine for an attribute. The esc_url() function does provide some additional options for controlling invalid protocols, but in terms of XSS, it’s not doing anything more than esc_attr() already accomplishes. An attribute, is an attribute, and an attribute should always be cleaned before display, regardless of what data we might expect it to contain. The esc_attr() function prevents XSS.

All of that being said, I appreciate you bringing this up. We’ll certainly make an effort to fully conform with WordPress standards by applying esc_url() when working with $_SERVER['REQUEST_URI']. I’m adding this to our list now, so we can have this updated for the next maintenance release. For the sake of conformity, I agree this should be implemented if at all possible.

Please let us know if anything else comes up. We’ll be happy to review it with you.

Viewing 20 replies - 1 through 20 (of 20 total)

This topic is closed to new replies. Topics with no replies for 2 weeks are closed automatically.

Old Forums (READ-ONLY): The community now lives at WP Sharks™. If you have an s2Member® Pro question, please use our new Support System.

Contacting s2Member: Please use our Support Center for bug reports, pre-sale questions & technical assistance.