fix possible sql injection issue

[oaxlin]

No description provided.

[oaxlin]

While this covers the 4x branch... the other branches probably need it as well.

[freeside]

Hi,

As far as I can tell, this is not an actual SQL injection issue that needs to be fixed. This internal subroutine is not passed untrusted input by the user interface.

-Ivan

[oaxlin]

It shouldn't matter that it's possible to exploit in actual practice. Especially when the fix is so simple. It's just good programming practice to fix these types of problems.

Especially since this is the highest ranked issue on the OWASP top 10
https://www.owasp.org/index.php/Top_10_2013-Top_10

As a software that should be striving for the highest levels of PCI compliance, I would expect that a patch like this would be heavily valued.

This makes little Bobby Tables cry. https://xkcd.com/327/

[freeside]

This is not an SQL injection vulnerability, actual or theoretical. it is a patch to randomly add quoting to a method WHICH NEVER RECEIVES UNTRUSTED USER INPUT. I don't see how this can be described as "good programming practice".

Throwing around links about the general problem and insinuating that we're not practicing good security or not PCI compliant instead of discussing the actual technical issues is not useful discussion.

SQL injection concerns user input. This does not.

[oaxlin]

NEVER is a bit strong.

Here is proof that I have successfully done the supposedly impossible. If you open this URI in your browser the apache will execute the private method in an unexpected manner which never properly validates the agent data.

/elements/tr-select-cust-part_pkg.html?cust_main=FS::CurrentUser::BootstrapUser

And it successfully runs an SQL query on the database.

[freeside]

I don't understand what "Proof" you are attempting to convey. This URL does not illustrate an SQL injection via the method you have patched. Perhaps you have modified FS::part_pkg sub curuser_pkgs_sql and elements/select-part_pkg.html ?

It does, however, point out that direct fetching of Mason elements is a possible attack surface. There could be other issues there. Thanks for the heads-up. Fixed on all branches.

[oaxlin]

Glad to see one security issue is fixed. The mason elements are full of hacking joy... I found a way to totally bring down all of freeside from within the mason elements. I will have to give you details on that in a private email. You may want to simply 403 forbidden the entire elements directory trees, if that is feasible.

However the proof I gave above was more to point out that an attack is possible. Just because it shouldn't happen doesn't mean it cannot happen. Saying it is not possible as a reason for omitting good programming practices will eventually come back to haunt you.

All SQL data should be properly quoted. Not just "user data". At some point, or by some other exploit (like the one I showed above) a bad person may be able to get into the private method. Just like I did.

Closing the mason exploit is a fantastic first step. But that doesn't mean another hole doesn't exist to gain access to the private method inappropriately. Remember, the freeside code is readily available for anyone to read, this gives would be attackers great opportunity to craft extremely complex attacks.

All this and more is why "private" methods, including ones that aren't intended for "user data" are potential candidates for SQL injection.

[freeside]

You may want to simply 403 forbidden the entire elements directory trees, if that is feasible.

Yes, I did.

However the proof I gave above was more to point out that an attack is possible. Just because it
shouldn't happen doesn't mean it cannot happen. Saying it is not possible as a reason for
omitting good programming practices will eventually come back to haunt you.

I agree with this statement, but let's keep the discussion on the specifics and not engage in gratuitous soapboxing. I don't think this applies to this specific case, which is a helper method for combining integers.

All SQL data should be properly quoted. Not just "user data". At some point, or by some other
exploit (like the one I showed above) a bad person may be able to get into the private method.
Just like I did.

No, you did not.

In the hypothetical situation where an attacker (with employee credentials) can "get into private methods", then our goose is completely cooked anyway.

But that doesn't mean another hole doesn't exist to gain access to the private method
inappropriately.

Actually, I audited carefully all uses of this method when you first sent in the patch, and no, there isn't.

Thanks again for your input.

[oaxlin]

I don't think this applies to this specific case, which is a helper method for combining integers.

However, the method doesn't guarantee that the data it receives is, in fact, an integer.

I and the dozen or so peers have discussed this pull request outside of this thread. Everyone I've talked with, aside from yourself, maintains the proper coding practice is to be safe, and ALWAYS quote. These peers include a couple organizers/speakers of state wide security conference know as SAINTCON.

One of the speakers even mentioned that this discussion would make a good topic for a SAINTCON 2016 presentation. The concept of protecting, quoting and verifying all data. Not just data that originates from a user. And why it is important.

If it would help. I could ask some of them to weigh in on the topic directly, rather than taking my word for it.

And yes I know it's silly to spend so much effort for a 40 character pull request. But your data, my data and the data of our users deserves to be protected as best as it can.

[freeside]

However, the method doesn't guarantee that the data it receives is, in fact, an integer.

As previously discussed at length, this internal method is not an attack surface. Conflating it with "SQL injection" is an unfair characterization of a non-security issue.

Your colleagues and their security conference are not on-topic.

I agree that security is important and that our users deserve careful consideration and discussion of security issues. At this time, I believe our time would be more well-spent finding and fixing actual problems or infrastructure than belaboring this point.

Thanks again, I very much appreciate your input and look forward to your additional patches and suggestions.

[oaxlin]

Here is a document on the OWASP site that may help.
https://www.owasp.org/images/6/6c/Encoded_Attacks_Threats_Countermeasures_9_30_08.pdf

Page 22 describes "trust boundaries" as does this link
https://www.owasp.org/index.php/Identify_resources_and_trust_boundaries

According to page 30
Integrity checks must be included wherever data passes from a trusted to a less trusted boundary.

Specific example:

The current expected data flow in freeside goes like this

1. WEB httemplate/misc/change_pkg.cgi
2. WEB tr-select-cust-part_pkg.html
3. APP FS::part_pkg->agent_pkgs_sql
4. APP FS::part_pkg->_pkgs_sql
5. DATA Database

I would like to propose that freeside has 3 trust boundaries in this example. Which I have named simply WEB, APP, and DATA as shown above. This is a fairly common trust model.

However, one could argue that there is no APP layer for freeside, in which case you would still have both WEB and DATA trust boundaries.

This patch provides at least minimal validation between the database trust layer and the layer above it.

[freeside]

Jason,

This is not an appropriate forum for general discussion of Freeside's security architecture. This is a pull request for a specific change, which has been already discussed at length.

If you would like to discuss Freeside's security architecture in general, please subscribe to the freeside-devel mailing list. If you have a specific security issue (which, I reiterate for the record, this pull request does is not), please contact us privately.

Thank you again for your input.