The Services 3.0 release exposes a number of methods which include an optional pagesize parameter. If a site is configured with certain permissions for anonymous user (e.g. "access content"), it is possible for an unauthenticated user to DOS a site with a large number of nodes by sending requests to the node index method with a large pagesize argument. The response to this request would be the entire contents of the node table in any format supported by Services.

The attached patches against 7.x-3.x (thanks, marcingy) and 6.x-3.x fix this by adding a "perform unlimited index queries" permission which restricts unlimited index queries to users with the appropriate permission, and add a set of variables which set the maximum page size for unauthenticated users.

CommentFileSizeAuthor
#12 0001-testing.patch11.71 KBkylebrowning
#11 0001-testing.patch23.09 KBkylebrowning
#8 pagesize-permission-6-tests.patch10.97 KBmarcingy
#7 pagesize-permission-6.tests_.patch10.97 KBmarcingy
#5 pagesize-permission-7-tests.patch8.66 KBmarcingy
#4 pagesize-permission-7-tests.patch7.99 KBmarcingy
#2 pagesize-permission-7-tests.patch6.61 KBmarcingy
pagesize-permission-7.x.patch5.05 KBcotto
pagesize-permission-6.x.patch5.89 KBcotto

Comments

marcingy’s picture

marcingy commented
Priority: Normal » Major

Will try and look at this over the weekend and add tests as suggested by Yuri.

marcingy’s picture

marcingy commented
StatusFileSize
new pagesize-permission-7-tests.patch6.61 KB

Patch with tests to ensure that only users with appropriate permission can retrieve more nodes than the default.

Status: Needs review » Needs work

The last submitted patch, pagesize-permission-7-tests.patch, failed testing.

marcingy’s picture

marcingy commented
Status: Needs work » Needs review
StatusFileSize
new pagesize-permission-7-tests.patch7.99 KB

So with the failing test why are we checking the number of services permissions - totally and utterly pointless....achives nothing what so ever. Adding to this patch to get rid of said test.

marcingy’s picture

marcingy commented
StatusFileSize
new pagesize-permission-7-tests.patch8.66 KB

More code can killed

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Reviewed & tested by the community

I think #5 is good to be committed.

marcingy’s picture

marcingy commented
Version: 7.x-3.x-dev » 6.x-3.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new pagesize-permission-6.tests_.patch10.97 KB

D6 version, will commit if the bot likes it

marcingy’s picture

marcingy commented
StatusFileSize
new pagesize-permission-6-tests.patch10.97 KB
marcingy’s picture

marcingy commented

Bot doesn't seem to want to play this needs manual review.

kylebrowning’s picture

kylebrowning commented
Status: Needs review » Reviewed & tested by the community

Looks good.

kylebrowning’s picture

kylebrowning commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 0001-testing.patch23.09 KB

lets see if drupal likes this patch.

kylebrowning’s picture

kylebrowning commented
StatusFileSize
new 0001-testing.patch11.71 KB

fucking drupal bot.

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Needs work

Looks fine to me, but I was not able to apply the patch. Kyle could you please reroll?

kylebrowning’s picture

kylebrowning commented

Yeah the patch doesn't apply after you commit a coder change to the whole codebase :P

marcingy’s picture

marcingy commented
Status: Needs work » Fixed

Committed thanks.

kylebrowning’s picture

kylebrowning commented
Status: Fixed » Closed (fixed)