Skip to content
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

com() has been disabled for security reasons #79

Closed
Otto42 opened this issue Dec 9, 2015 · 18 comments
Closed

com() has been disabled for security reasons #79

Otto42 opened this issue Dec 9, 2015 · 18 comments

Comments

@Otto42
Copy link

Otto42 commented Dec 9, 2015

After the release of 4.4, we received this report in the forums:

https://wordpress.org/support/topic/issue-after-updating-to-44

Warning: com() has been disabled for security reasons in D:...\wordpress\wp-includes\random_compat\random.php on line 94

Looks like some servers disable this via the INI file. Maybe a check to prevent a warning here is in order.

@marco-acorte
Copy link

me too... my issue occur on windows 2012 hosting IIS 8.5
visible here... http://www.acor3.it/

@paragonie-scott
Copy link
Member

Is there a specific INI directive being used to block COM that we can detect?

@rchouinard
Copy link

You should be able to parse the disable_functions using ini_get(). I'm not sure if there is a better way to detect it or not.

@paragonie-scott
Copy link
Member

5f727f8 can anyone confirm this fixes the issue?

@dd32
Copy link
Contributor

dd32 commented Dec 9, 2015

Looks like that might fix it (haven't tested), but it'll also need to check case-insensitive.

@paragonie-scott
Copy link
Member

Oh, right. I hadn't even considered that.

@paragonie-scott
Copy link
Member

@narfbg
Copy link

narfbg commented Dec 10, 2015

Um ... this can't possibly work, because the setting is called 'disable_classes' (only a single 'd' in it).

Also, explode() with a comma won't work in some cases, because if you're anything like me, you'd put a space after each comma. So you may end up with e.g. array('com', ['dom', ' com'])), which returns false. I'd suggest the following:

$RandomCompat_disabled_classes = preg_split('#\s*,\s*#', strtolower(ini_get('disable_classes')));

As a side note, I always use in_array()'s third parameter to enforce compare by type. The call here isn't affected because both ini_get() and explode() ensure you'll get a string, but it's good to get in the habit of using it as in_array('com', [0]) would return true.

@Otto42
Copy link
Author

Otto42 commented Dec 10, 2015

This disabled check seems a bit like it needs an actual proper check, spelled out explicitly, rather than some half-checks and resorting to regular expressions.

Something like this might help spell it out a bit clearer. Rename the function or inline the basic logic of it, if you like. But it should be spelled out so as to be clear what is occurring.

// input: class name
// output: true if class exists and is enabled, false otherwise
function is_class_disabled( $class ) {
    $disabled_classes = @ini_get("disable_classes");
    if (empty($disabled_classes) == false) {
        $disabled_classes = explode(',', $disabled_classes);
        $disabled_classes = array_map('trim', $disabled_classes);
        $disabled_classes = array_map('strtolower', $disabled_classes);
        return (class_exists($class) == true && array_search($class, $disabled_classes) === false);
    }
    return class_exists($class);
}

@narfbg
Copy link

narfbg commented Dec 10, 2015

You say that like using regular expressions is a bad thing ... they are not evil and this one is dead simple.

@paragonie-scott
Copy link
Member

I agree that having an explicit check would be a good thing in general, but writing a helper function that will only, in our library, ever be used exactly once isn't really that helpful. It doesn't reduce code duplication at all.

I tagged 1.1.4 with your suggested fix @narfbg.

@narfbg
Copy link

narfbg commented Dec 10, 2015

Great. Although, you may want to reconsider doing quick releases in that fashion and allow a few days time for peer review. :)

@rchouinard
Copy link

I agree with @narfbg. Release tags are being made a little too quickly. It would be a good idea to hold off tagging future releases until bug fixes/changes are verified.

@paragonie-scott
Copy link
Member

Noted, I'll be less trigger happy about patch version releases in the future.

@paragonie-scott
Copy link
Member

Does the latest patch mitigate this issue completely?

@dd32
Copy link
Contributor

dd32 commented Dec 22, 2015

@paragonie-scott Although I've not tested it directly on a COM-enabled server, I've been unable to find a flaw in the logic, if that helps at all.

@marco-acorte
Copy link

problem fixed. tested on COM-enabled server (windows 2012 hosting IIS 8.5)

@paragonie-scott
Copy link
Member

Excellent news.

Thanks @Otto42 for reporting this and everyone for testing the fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants