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

manage_pasteObjects breaks when pasting many objects #217

Closed
icemac opened this Issue Nov 1, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@icemac
Member

icemac commented Nov 1, 2017

_cb_decode only allows 8 kB of compressed copy data.
This was introduced in 6ca3691 to prevent a DoS attack.

The curent implementation also prevents pasting many objects in file system code be calling e.g.:

dest.manage_pasteObjects(source.manage_copyObjects(ids))

I propose a new parameter to manage_pasteObjects to override the maxsize value of _cb_decode to support this use case again or to use a different value if cb_copy_data is not None when calling manage_pasteObjects.

What do you think of my proposal?

This issue exists on Zope 2.13 and Zope 4.

See also https://stackoverflow.com/questions/17581962/copyerror-ofs-copysupport-manage-pasteobjects-limited-to-160-objects

@tseaver

This comment has been minimized.

Member

tseaver commented Nov 1, 2017

ISTM that allowing TTW code to override that count is a Bad Thing (TM).

Maybe split the methods into the TTW-callable manage_{copy,paste}Objects and an underlying _{copy,paste}Objects which takes that flag?

@jan-jockusch

This comment has been minimized.

Contributor

jan-jockusch commented Nov 5, 2017

In our use cases, using the TTW interface to move massive amounts of code via copy/paste is a normal thing in Zope. I'd have a hard time telling my users that they're attempting DoS in version 4 with an action that was totally OK in version 2.13.
If this is a serious concern, can we make the feature at least configurable, please? I can attempt a patch (since I'll have to make sure this works for my installations anyway).

@hannosch

This comment has been minimized.

Member

hannosch commented Nov 5, 2017

We could also make the default size a zope.conf option and look it up from there.

I think gunicorn's limit-request-field-size (http://docs.gunicorn.org/en/stable/settings.html#limit-request-field-size) and waitress' max_request_header_size (https://docs.pylonsproject.org/projects/waitress/en/latest/arguments.html) serve a similar purpose here. Those apply length constraints on the size of all headers or on each header. A per-header upper length limit would already limit the size of the Set-Cookie header, so maybe this would be enough here?

Otherwise we could make it an additional config option, specific to zlib decompression.

@icemac

This comment has been minimized.

Member

icemac commented Nov 7, 2017

@jan-jockusch This problem only will strike you if you select many, many objects in a folder and copy them to another place. I do not think there is a problem when copying TTW as copying a folder needs only the reference (physical path) to the folder but not to each of its contained objects in the REQUEST.

The behaviour of _cb_decode described in this issue did not change beetween Zope 2.13 and Zope 4. It was introduced in 2.13.21 as a security fix and I'd like to keep the security level introduced there.

@hannosch You suggestion sounds a bit like an overkill as the current limit can only be hit when copying many objects from a single folder (this is what we do programmatically where I hit this issue). I do not think that it will be a problem from TTW code as we have 8 kByte text containing the physical paths of the objects which should be copied excluding contents of folders.

I think the suggestion of @tseaver is most suitable for the problem.

@icemac

This comment has been minimized.

icemac added a commit that referenced this issue Apr 18, 2018

@icemac icemac removed the help wanted label Apr 18, 2018

@icemac icemac self-assigned this Apr 18, 2018

@icemac icemac added this to the 4.0b4 milestone Apr 18, 2018

@icemac icemac closed this in c3129e8 Apr 19, 2018

@icemac

This comment has been minimized.

Member

icemac commented Apr 19, 2018

Fix needs to be ported to 2.13, too.

@icemac icemac reopened this Apr 19, 2018

icemac added a commit that referenced this issue Apr 19, 2018

icemac added a commit that referenced this issue Apr 23, 2018

Add ability to paste objects without limits. [2.13] (#270)
* Add tests to check the DoS prevention code.

* Add `._pasteObjects()` to be able to paste objects without limits.

Fixes #217.

@icemac icemac modified the milestones: 4.0b4, 4.0b5 Apr 23, 2018

@icemac

This comment has been minimized.

Member

icemac commented Apr 23, 2018

Fixed via #269 (master) resp. #270 (2.13).

@icemac icemac closed this Apr 23, 2018

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