Skip to content

Make sure we don't allow IP overrides within the com_content vote feature#32452

Closed
zero-24 wants to merge 2 commits intojoomla:stagingfrom
zero-24:iphelper
Closed

Make sure we don't allow IP overrides within the com_content vote feature#32452
zero-24 wants to merge 2 commits intojoomla:stagingfrom
zero-24:iphelper

Conversation

@zero-24
Copy link
Copy Markdown
Contributor

@zero-24 zero-24 commented Feb 17, 2021

Pull Request for an issue reported to the JSST by DangKhai from Viettel Cyber Security

Summary of Changes

Make sure we dont allow IP overrides by the X-Forwarded-For or Client-Ip HTTP headers for the com_content vote feature.

Testing Instructions

Setup voting for an article
make sure voting works
apply the patch
make sure voting still works

Actual result BEFORE applying this Pull Request

It was possible to override the ip using the X-Forwarded-For or Client-Ip HTTP headers

Expected result AFTER applying this Pull Request

that behavior is disabled now

Documentation Changes Required

None

@zero-24 zero-24 added this to the Joomla! 3.9.25 milestone Feb 17, 2021
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 8b0103e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32452.

@sandewt
Copy link
Copy Markdown
Contributor

sandewt commented Feb 19, 2021

For the test I changed the following code in ...\libraries\vendor\joomla\utilities\src\IpHelper.php

public static function setAllowIpOverrides($newState)
{
    // self::$allowIpOverrides = $newState ? true : false; // original code
    self::$allowIpOverrides = $newState ? false : false; // changed code
}

The test remains successful.
In other words, does the concerned method do what it is supposed to do?

// Make sure we don't allow IP overrides
IpHelper::setAllowIpOverrides(false);


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32452.

@PhilETaylor

This comment was marked as abuse.

@brianteeman
Copy link
Copy Markdown
Contributor

and that will include joomla.org

@zero-24
Copy link
Copy Markdown
Contributor Author

zero-24 commented Feb 19, 2021

Understood. I'm happy to hear proposals to fix this issue. We might have to do some kind of configuration where we get the info from that we run behind that proxy setup?

@PhilETaylor

This comment was marked as abuse.

@sandewt
Copy link
Copy Markdown
Contributor

sandewt commented Feb 19, 2021

Understood. I'm happy to hear proposals to fix this issue.

Maybe you can do something with this, see:

if (!filter_var($ip, FILTER_VALIDATE_IP))

@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 15, 2021
@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Copy Markdown
Contributor Author

zero-24 commented Mar 25, 2021

Its about bypassing the ip restrictions on voting by bypassing it using the mentiond headers.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@zero-24
Copy link
Copy Markdown
Contributor Author

zero-24 commented Mar 25, 2021

see: #32866

@zero-24 zero-24 closed this Mar 25, 2021
@zero-24 zero-24 deleted the iphelper branch March 25, 2021 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Updates Requested Indicates that this pull request needs an update from the author and should not be tested.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants