This repository has been archived by the owner. It is now read-only.

Count modules security patch #1307

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@ghost

ghost commented Jun 22, 2012

Eliminates unneeded eval call and checks for valid operators

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Jun 22, 2012

Contributor

You have a few code style issues but the report isn't up yet. You'll see it on http://developer.joomla.org/pulls/index.html when it has run.

Contributor

realityking commented Jun 22, 2012

You have a few code style issues but the report isn't up yet. You'll see it on http://developer.joomla.org/pulls/index.html when it has run.

+
+ // $words must be odd, an even number of words is a mistake so skip processing
+ if (!(count($words) & 1)) {
+ return false;

This comment has been minimized.

@realityking

realityking Jun 22, 2012

Contributor

I'd throw an InvalidArgumentException here.

@realityking

realityking Jun 22, 2012

Contributor

I'd throw an InvalidArgumentException here.

This comment has been minimized.

@ghost

ghost Jun 23, 2012

From deploying Joomla! CMS, I'd rather not have an exception thrown which breaks the website due to some invalid template...especially if it was in an admin template which then made it impossible to fix without directly updating database.

@ghost

ghost Jun 23, 2012

From deploying Joomla! CMS, I'd rather not have an exception thrown which breaks the website due to some invalid template...especially if it was in an admin template which then made it impossible to fix without directly updating database.

This comment has been minimized.

@realityking

realityking Jun 23, 2012

Contributor

I agree that we can't do it in 2.5. But throwing an exception will make it much easier for devs to even notice their error so I think that is preferable for 3.0.

@realityking

realityking Jun 23, 2012

Contributor

I agree that we can't do it in 2.5. But throwing an exception will make it much easier for devs to even notice their error so I think that is preferable for 3.0.

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Jun 22, 2012

Contributor

What do others think about deprecating the use operators in countModules()? Templates can use them right in their code, it's only a tiny bit of more code for them and would save us the headache of the eval().

Contributor

realityking commented Jun 22, 2012

What do others think about deprecating the use operators in countModules()? Templates can use them right in their code, it's only a tiny bit of more code for them and would save us the headache of the eval().

@elinw

This comment has been minimized.

Show comment
Hide comment
@elinw

elinw Jun 23, 2012

Contributor

@realityking I'm with you on the operators.

Contributor

elinw commented Jun 23, 2012

@realityking I'm with you on the operators.

@elinw

This comment has been minimized.

Show comment
Hide comment
@elinw

elinw Jun 23, 2012

Contributor

I want to mention to possible readers of this issue that despite the title this is not a security issue except in the sense that webmasters can hack their own sites by editing files and entering malicious code. It is absolutely the case that if you give someone privileges to edit any file,including your template files, and they chose to destroy your site, they can. That's why you should not give those privileges to anyone you cannot trust completely.

About the exception, I would only even be considering these changes for CMS 3.0 anyway so that should be documented as a b/c change and we should take advantage of the fact that templates have a lot to deal with in 3.0 anyway.

Contributor

elinw commented Jun 23, 2012

I want to mention to possible readers of this issue that despite the title this is not a security issue except in the sense that webmasters can hack their own sites by editing files and entering malicious code. It is absolutely the case that if you give someone privileges to edit any file,including your template files, and they chose to destroy your site, they can. That's why you should not give those privileges to anyone you cannot trust completely.

About the exception, I would only even be considering these changes for CMS 3.0 anyway so that should be documented as a b/c change and we should take advantage of the fact that templates have a lot to deal with in 3.0 anyway.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 23, 2012

Actually, anything which calls countModules on something will cause an eval on unfiltered, unchecked code. While it's true that the only item IN the CMS which does so is the template, this does not mean there aren't 3rd party extensions which call countmodules for some reason[for example, similar functionality to the loadModules] which would allow content editors to insert malicious code that goes through countModules....in short, a hidden eval with no security checks is a security hole...it may be an unexploited hole, but it is a hole nevertheless. I call a spade and spade, and a security hole a security hole.

ghost commented Jun 23, 2012

Actually, anything which calls countModules on something will cause an eval on unfiltered, unchecked code. While it's true that the only item IN the CMS which does so is the template, this does not mean there aren't 3rd party extensions which call countmodules for some reason[for example, similar functionality to the loadModules] which would allow content editors to insert malicious code that goes through countModules....in short, a hidden eval with no security checks is a security hole...it may be an unexploited hole, but it is a hole nevertheless. I call a spade and spade, and a security hole a security hole.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 23, 2012

@realityking: I'm not seeing any codestyle issues...or more accurately I'm not seeing any NEW issues in pull request. I'm for eliminating the eval call altogether, but figured I'd keep this code pull backwards compatible and only deal with issues of ensuring operators are really operators, checking parameter counts, and getting rid of eval for 90% of the calls by removing the call when there's only 1 word.

ghost commented Jun 23, 2012

@realityking: I'm not seeing any codestyle issues...or more accurately I'm not seeing any NEW issues in pull request. I'm for eliminating the eval call altogether, but figured I'd keep this code pull backwards compatible and only deal with issues of ensuring operators are really operators, checking parameter counts, and getting rid of eval for 90% of the calls by removing the call when there's only 1 word.

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Jun 23, 2012

Contributor

@garyamort
The pull tester agrees with me and sees 6 code style issues ;) http://developer.joomla.org/pulls/pulls/1307.html

As for the security or not security debate: If you're passing user input to that function it's your responsibility to filter it. If it was considered a security issue just checking for an uneven number of terms wouldn't be a complete solution either. Anyway, it's not treated as a security issue but we still wanna fix it so I don't think it's a big deal.

Contributor

realityking commented Jun 23, 2012

@garyamort
The pull tester agrees with me and sees 6 code style issues ;) http://developer.joomla.org/pulls/pulls/1307.html

As for the security or not security debate: If you're passing user input to that function it's your responsibility to filter it. If it was considered a security issue just checking for an uneven number of terms wouldn't be a complete solution either. Anyway, it's not treated as a security issue but we still wanna fix it so I don't think it's a big deal.

@realityking

This comment has been minimized.

Show comment
Hide comment
@realityking

realityking Jun 23, 2012

Contributor

@garyamort
The pull tester agrees with me and sees 6 code style issues ;) http://developer.joomla.org/pulls/pulls/1307.html

As for the security or not security debate: If you're passing user input to that function it's your responsibility to filter it. If it was considered a security issue just checking for an uneven number of terms wouldn't be a complete solution either. Anyway, it's not treated as a security issue but we still wanna fix it so I don't think it's a big deal.

Contributor

realityking commented Jun 23, 2012

@garyamort
The pull tester agrees with me and sees 6 code style issues ;) http://developer.joomla.org/pulls/pulls/1307.html

As for the security or not security debate: If you're passing user input to that function it's your responsibility to filter it. If it was considered a security issue just checking for an uneven number of terms wouldn't be a complete solution either. Anyway, it's not treated as a security issue but we still wanna fix it so I don't think it's a big deal.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jun 25, 2012

@realityking: I agree about just checking for uneven terms...that's why I added a pregmatch on the operators to make sure their in the set of operators. The operands will always be turned into integers, so their safe, but the operands are passed to eval as is. I'll look at those checkstyle issues later today....slowly learning how to read that report.

ghost commented Jun 25, 2012

@realityking: I agree about just checking for uneven terms...that's why I added a pregmatch on the operators to make sure their in the set of operators. The operands will always be turned into integers, so their safe, but the operands are passed to eval as is. I'll look at those checkstyle issues later today....slowly learning how to read that report.

@LouisLandry

This comment has been minimized.

Show comment
Hide comment
@LouisLandry

LouisLandry Oct 9, 2012

Contributor

@garyamort my apologies for us not getting this all sorted out sooner. It'd be fantastic if you could rebase (cannot be merged) this and add the unit tests from #1303 into this pull request so that we get the tests and changes all in one go. Once you get that done we can get this thing one last check and merged into the platform. I really appreciate the effort and initiative working this problem. It would also be great if you could provide a little more descriptive title/description on the pull request for our changelog.

I'm marking this for the 12.3 release. Additionally I'm closing it for now, but not because it is rejected. It isn't in a state where it can be reviewed further or merged so I'm closing it so that it doesn't stay in our review queue. Once you get things cleaned up please re-open it and we'll get it sorted out. Thanks!

Contributor

LouisLandry commented Oct 9, 2012

@garyamort my apologies for us not getting this all sorted out sooner. It'd be fantastic if you could rebase (cannot be merged) this and add the unit tests from #1303 into this pull request so that we get the tests and changes all in one go. Once you get that done we can get this thing one last check and merged into the platform. I really appreciate the effort and initiative working this problem. It would also be great if you could provide a little more descriptive title/description on the pull request for our changelog.

I'm marking this for the 12.3 release. Additionally I'm closing it for now, but not because it is rejected. It isn't in a state where it can be reviewed further or merged so I'm closing it so that it doesn't stay in our review queue. Once you get things cleaned up please re-open it and we'll get it sorted out. Thanks!

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