Variable help

[mickjav]

I have the following Line

Code Select Expand

LIMIT '. $_GET['start'] . ', 20');
I wish to replace the 20 with a Variable $ddperpage

Thanks In Advance mick

[Arantor]

Where exactly is this line?

Depending on the line you may have a security hole going on...

[mickjav]

Thanks That's the whole line It's part of a SQL Statement For Paging,

Once This part is complete I'll be posting the Code for the mod.

[Arantor]

It's the last part of a statement. I need to see the whole statement.

If that's the current code, it could possibly see your site hacked.

[mickjav]

I've created a variable

$ddperpage=20;

Which is used in a number of places within a function

The SQL Is

Code Select Expand

// Create the page index
    $context['page_index_top'] = constructPageIndex($scripturl . '?action=gallery;cat=' . $cat . (isset($_REQUEST['searchresult']) && isset($_REQUEST['searchcat']) && !empty($_REQUEST['searchcat']) ? ';searchresult;searchcat=' . $_REQUEST['searchcat'] : ''), $_GET['start'], $cats_total , $ddperpage);

    //List all the categories
    $dbresult = $smcFunc['db_query']('', '
        SELECT
            c.id_cat, c.title, p.view, c.roworder, c.description, c.image, c.filename, c.total,
            l.id_picture, l.title pictitle, l.date, m.id_member, m.real_name, c.redirect, mg.online_color
        FROM {db_prefix}gallery_cat AS c
            LEFT JOIN {db_prefix}gallery_catperm AS p ON (p.id_group = '. $groupid . ' AND c.id_cat = p.id_cat)
            LEFT JOIN {db_prefix}gallery_pic AS l ON (c.LAST_id_picture = l.id_picture)
            LEFT JOIN {db_prefix}members AS m ON (m.id_member = l.id_member)
            LEFT JOIN {db_prefix}membergroups AS mg ON (mg.ID_GROUP = IF(m.ID_GROUP = 0, m.ID_POST_GROUP, m.ID_GROUP))

        WHERE c.id_parent = '.  $cat . (isset($_REQUEST['searchresult']) && isset($_REQUEST['searchcat']) && !empty($_REQUEST['searchcat']) ? ' AND c.title LIKE "%' . $_REQUEST['searchcat'] . '%"' : '') . '
        GROUP BY c.id_cat
        ORDER BY c.title ASC
        LIMIT '. $_GET['start'] . ', 20');
I have include another location where the variable has been used above the SQL

[Arantor]

Ah, so there's only two SQL injections to deal with. I'll pick this up when I'm done with work for the day...

[mickjav]

OK Thanks But that is also used to hide/display the Paging Links

Code Select Expand


If ($cats_total > $ddperpage || isset($_REQUEST['searchresult'])) {

And

If ($cats_total > $ddperpage) {

This is what it relates too:

https://www.databasedreams.co.uk/charts/index.php?action=gallery;cat=35

[Arantor]

Code Select Expand

    // Create the page index
    $searchcat = isset($_REQUEST['searchcat']) && !empty($_REQUEST['searchcat']) ? $_REQUEST['searchcat'] : '';
    $context['page_index_top'] = constructPageIndex($scripturl . '?action=gallery;cat=' . $cat . ($searchcat ? ';searchresult;searchcat=' . urlencode($searchcat) : ''), $_REQUEST['start'], $cats_total, $ddperpage);

    //List all the categories
    $dbresult = $smcFunc['db_query']('', '
        SELECT
            c.id_cat, c.title, p.view, c.roworder, c.description, c.image, c.filename, c.total,
            l.id_picture, l.title pictitle, l.date, m.id_member, m.real_name, c.redirect, mg.online_color
        FROM {db_prefix}gallery_cat AS c
            LEFT JOIN {db_prefix}gallery_catperm AS p ON (p.id_group = {int:group_id} AND c.id_cat = p.id_cat)
            LEFT JOIN {db_prefix}gallery_pic AS l ON (c.LAST_id_picture = l.id_picture)
            LEFT JOIN {db_prefix}members AS m ON (m.id_member = l.id_member)
            LEFT JOIN {db_prefix}membergroups AS mg ON (mg.ID_GROUP = IF(m.ID_GROUP = 0, m.ID_POST_GROUP, m.ID_GROUP))

        WHERE c.id_parent = {int:cat}' .  ($searchcat ? ' AND c.title LIKE {string:searchcat}' : '') . '
        GROUP BY c.id_cat
        ORDER BY c.title ASC
        LIMIT {int:start}, {int:limit}',
        array(
            'group_id' => $groupid,
            'cat' => $cat,
            'searchcat' => '%' . str_replace('%', '', $searchcat) . '%',
            'start' => $_REQUEST['start'],
            'limit' => $ddperpage,
        )
    );

Give that a go, no more SQL injections, no more reflected XSS attacks and a tidier query to boot.

The other instances you're talking about are not shoving variables into a database query without sanitising them first and so are not vulnerable to anything.

[mickjav]

Sorry The Paging No Longer works It just stays on first page

[mickjav]

Also The Search no longer displays results I just shows the first page.

But the page count and paging links update correcly 

[Arantor]

Well, it's not like I have a lot of information to work with seeing how I have no idea where you're actually using it (and I don't have SMF Gallery), like I don't know what you've actually passed into $cats_total, $cat, $groupid or half the variables being used here, I've just taken what you wrote and restructured it to behave like normal SMF code...

So what do the links actually generate as?

[mickjav]

Sorry for any misunderstanding.

Found The Problem You used $_REQUEST When the old version used $_GET Seems to work a treat now and I learnt a bit more thanks mick

[mickjav]

Lol Spoke too soon

The Paging works but not the Search,

Included all to end of query.

Code Select Expand

   global $txt, $smcFunc, $scripturl, $modSettings, $subcats_linktree, $user_info, $context, $gallerySettings;

$ddperpage=20;

    if ($context['user']['is_guest'])
        $groupid = -1;
    else
        $groupid =  $user_info['groups'][0];

    // Count all the categories
    $dbquery = $smcFunc['db_query']('', '
        SELECT c.id_cat, c.title
    FROM {db_prefix}gallery_cat AS c
    WHERE c.id_parent = '. $cat . (isset($_REQUEST['searchresult']) && isset($_REQUEST['searchcat']) && !empty($_REQUEST['searchcat']) ? ' AND c.title LIKE "%' . $_REQUEST['searchcat'] . '%"' : ''));

    $cats_total = $smcFunc['db_num_rows']($dbquery);
    $smcFunc['db_free_result']($dbquery);

    // Create the page index
    $context['page_index_top'] = constructPageIndex($scripturl . '?action=gallery;cat=' . $cat . (isset($_REQUEST['searchresult']) && isset($_REQUEST['searchcat']) && !empty($_REQUEST['searchcat']) ? ';searchresult;searchcat=' . $_REQUEST['searchcat'] : ''), $_GET['start'], $cats_total , $ddperpage);

    //List all the categories
    $dbresult = $smcFunc['db_query']('', '
        SELECT
            c.id_cat, c.title, p.view, c.roworder, c.description, c.image, c.filename, c.total,
            l.id_picture, l.title pictitle, l.date, m.id_member, m.real_name, c.redirect, mg.online_color
        FROM {db_prefix}gallery_cat AS c
            LEFT JOIN {db_prefix}gallery_catperm AS p ON (p.id_group = {int:group_id} AND c.id_cat = p.id_cat)
            LEFT JOIN {db_prefix}gallery_pic AS l ON (c.LAST_id_picture = l.id_picture)
            LEFT JOIN {db_prefix}members AS m ON (m.id_member = l.id_member)
            LEFT JOIN {db_prefix}membergroups AS mg ON (mg.ID_GROUP = IF(m.ID_GROUP = 0, m.ID_POST_GROUP, m.ID_GROUP))

        WHERE c.id_parent = {int:cat}' .  ($searchcat ? ' AND c.title LIKE {string:searchcat}' : '') . '
        GROUP BY c.id_cat
        ORDER BY c.title ASC
        LIMIT {int:start}, {int:limit}',
        array(
            'group_id' => $groupid,
            'cat' => $cat,
            'searchcat' => '%' . str_replace('%', '', $searchcat) . '%',
            'start' => $_GET['start'],
            'limit' => $ddperpage,
        )
    );

[mickjav]

Think I have it now

Code Select Expand

WHERE c.id_parent = {int:cat}' .  ($_REQUEST['searchcat'] ? ' AND c.title LIKE {string:searchcat}' : '') . '
        GROUP BY c.id_cat
        ORDER BY c.title ASC
        LIMIT {int:start}, {int:limit}',
        array(
            'group_id' => $groupid,
            'cat' => $cat,
            'searchcat' => '%' . str_replace('%', '', $_REQUEST['searchcat']) . '%',
I replaced the Variable $searchcat For $_REQUEST['searchcat'], Hope that's OK for security Etc

all the best mick

[Arantor]

I specifically used $_REQUEST['start'] instead of the $_GET version because I know that SMF internally sanitises this and makes it safe to use.

I do see one bug I introduced, related to setting $searchcat because I wanted to strip out unnecessary checks (you don't need to do isset and !empty because the latter includes the former) so that should have been isset on searchresult and !empty on searchcat to assign $_REQUEST['searchcat'] to the shorthand variable so it's always set and you don't have to check it's set before using it (unlike your reversion which will likely throw errors because there's no guarantee that $_REQUEST['searchcat'] is defined)

But actually, you know what, I see similar security errors in the rest of your code and you can fix that yourself. I was trying to help and I would have if you'd given me the time to actually review what all your code was doing before you decided to go change things that I don't think you understand the ramifications of, I'd have actually figured it all out for you.

[mickjav]

Sorry But I only noticed why It wasn't working I'm a 25+ year VBA Developer Trying to learn PHP,

I asked for help because I Wasn't sure what I was doing which I thank you for but If you had asked me to post the whole Function I would have as I said I did intend making it available to all anyway.

If you see security problems please point them out so I can fix them or arrange for somebody to undertake the works

Sorry if I came across as a Know it all or something like that, You should note another member on this site was paid to Write that code All I'm doing is trying to learn from it, now your telling me what he did wasn't up to Security standards???

mick

[hungarianguy]

Sorry But I only noticed why It wasn't working I'm a 25+ year VBA Developer Trying to learn PHP,

I asked for help because I Wasn't sure what I was doing which I thank you for but If you had asked me to post the whole Function I would have as I said I did intend making it available to all anyway.

If you see security problems please point them out so I can fix them or arrange for somebody to undertake the works

Sorry if I came across as a Know it all or something like that, You should note another member on this site was paid to Write that code All I'm doing is trying to learn from it, now your telling me what he did wasn't up to Security standards???

mick

What mod is that? I have bought mods from several devs here and it is concerning that their code has security problems? I read your posts above and the code you posted says gallery. Is it the gallery pro mod?

[mickjav]

Yes It's gallery Pro But you should note I think He Is refereeing to my Edits OR edits I paid for.

I have full confidence in both gallery pro and also the developer I use for my mods.

I am still waiting for him to give a detailed error report so I can eval the code further.

[hungarianguy]

I havent bought that mod but its developer is strong so no worries.