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

[6.x] Limit expected bindings v2 #35972

Merged
merged 3 commits into from
Jan 21, 2021
Merged

[6.x] Limit expected bindings v2 #35972

merged 3 commits into from
Jan 21, 2021

Conversation

KaneCohen
Copy link
Contributor

@KaneCohen KaneCohen commented Jan 21, 2021

Update for recent security fix taking into account @mpyw comment here: #35865 (comment)

PR cleans inputs in case if passed values include nested arrays.

Copy link
Contributor

@mpyw mpyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KaneCohen I'll appreciate for your quick fix! Thanks

@driesvints driesvints changed the title Limit expected bindingx v2. [6.x] Limit expected bindings v2 Jan 21, 2021
@u01jmg3
Copy link
Contributor

u01jmg3 commented Jan 21, 2021

@KaneCohen: thanks for this. One thing I noticed, whereDay() and whereYear() don't make use of your new method scalarValue() but whereMonth() does. Was this on purpose?

@KaneCohen
Copy link
Contributor Author

KaneCohen commented Jan 21, 2021

@u01jmg3 updated. Looks like I've missed those when was porting code from 8.x branch.

Thanks for spotting.

@limingxinleo
Copy link

I think the following code is more reasonable

    /**
     * Returns scalar type value from an unknown type of input.
     *
     * @param  mixed  $value
     * @return mixed
     */
    protected function scalarValue($value)
    {
        if(is_array($value)){
           throw new \Exception(); 
       }
        return $value;
    }

@KaneCohen
Copy link
Contributor Author

@limingxinleo I think primary reason why this was not done in the first place is that throwing an exception might unnecessarily break existing applications which pass arrays as a value.

@@ -1593,7 +1604,7 @@ public function whereJsonLength($column, $operator, $value = null, $boolean = 'a
$this->wheres[] = compact('type', 'column', 'operator', 'value', 'boolean');

if (! $value instanceof Expression) {
$this->addBinding((int) $value);
$this->addBinding((int) $this->scalarValue($value));
Copy link
Contributor

@u01jmg3 u01jmg3 Jan 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary as we are casting to an integer? I guess it could still help if an array is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can’t really say. Casting an array into integer returns 1 - so there’s that. Really depends on what’s expected when one passes an array there.

@jaylinski
Copy link

@taylorotwell It would be nice if you could update this advisory accordingly: GHSA-3p32-j457-pg5x 🙏

@driesvints
Copy link
Member

Existing advisories can't be altered in GitHub unfortunately.

@DavidPrevot
Copy link

That’s fair: a CVE has already been assigned anyway. On the other hand, adding another advisory to document that new fix would be nice (as well as requesting another CVE for the incomplete previous fix).

@naderman
Copy link

naderman commented Feb 1, 2021

@driesvints @taylorotwell I see that you did publish a follow up blog post at https://blog.laravel.com/security-laravel-62012-7303-released, after the issue which got a CVE, because of a tagging problem. However the related(?) fix from this PR appears to only be in 6.20.14/7.30.4/8.24.0 - can you clarify whether you consider this part of the same issue and which versions should correctly be identified as vulnerable on packagist.org, github and other security scanning sites relying on the FriendsOfPHP/security-advisories database? The PR by @jaylinski there currently references a third party blog as the source, but I'd much rather reference an official blog post of yours if you could update it with the corresponding information? FriendsOfPHP/security-advisories#532 Thanks!

@stof
Copy link

stof commented Feb 1, 2021

@naderman https://blog.laravel.com/security-laravel-62012-7303-released is not about the fix in this PR. It is about the fact that the previous fix does not properly appear in the releases of illuminate/database before 7.30.3 (while it does for laravel/framework). This PR is a separate topic.
So I think we actually need 3 sets of advisories in FriendsOfPHP/security-advisories:

@driesvints
Copy link
Member

I'm currently checking if we can do a new security advisory. Please stay tuned.

@driesvints
Copy link
Member

@naderman @stof we published a second advisory for this specific fix: GHSA-x7p5-p2c9-phvg

@naderman
Copy link

naderman commented Feb 2, 2021

@driesvints thank you!

This was referenced Mar 12, 2021
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

Successfully merging this pull request may close these issues.

10 participants