WBCE CMS Forum

albatros

Mitglied

General settings / website footer

Hi all!

After lots of years thinking what to do with my WB-Websites, I want to make a change from WB to WBCE too. Glad I found you!

I have my first website-migration in progress and I want to insert something like

[== HTML ==]
<p>
Powered by 
<a target="_blank" href="http://www.wbce.org/">WBCE</a>
 and 
<a target="_blank" href="http://www.example.de">www.example.de</a>
</p>

into my footer. But after saving I have a lot of backslashes in there and the links in my frontend are wrong.

[== HTML ==]
<p>
Powered by 
<a target=\"_blank\" href=\"http://www.wbce.org/\">WBCE</a>
 and 
<a target=\"_blank\" href=\"http://www.example.de\">www.example.de</a>
</p>

And even better, with every click on the saving-button, there are more backslashes and more...
Problem could be found in a local xampp installation and on my webspace.

What am I doing wrong? Thanks a lot in advance!

Uwe / aka albatros

florian

Administrator

Re: General settings / website footer

Hi and welcome,

this sounds for me like a problem with magic_quotes_gpc = On (has to be off)

BR
-florian.

---

Wir Benötigen: Cents, Euros... jetzt spenden!

mrbaseman

Core-Team

Re: General settings / website footer

it might be a bug in one of the latest patches. see this thread

albatros

Mitglied

Re: General settings / website footer

Thanks a lot to both of you!
@Florian: magic_quotes_gpc sounded good to me, but after switching of, the problem still exists.
BTW: its the newest WBCE-version including the securitypatch.
So maybe mrbasemen, you are right.

I will make a try by changing the save.php later and report the result.

Beitrag geändert von albatros (02.03.2016 11:37:13)

florian

Administrator

Re: General settings / website footer

Are you using the CKE from the installer package?
forget it

Beitrag geändert von florian (02.03.2016 11:58:09)

---

Wir Benötigen: Cents, Euros... jetzt spenden!

mrbaseman

Core-Team

Re: General settings / website footer

I just could reproduce your problem in WB classic with the WBCE patch applied. The problem only appears in header and footer where html tags are allowed and only if you enclose the arguments of the tags with doublequotes, of course, otherwise neither add_slashes nor escapeString would change anything. Unfortunately, I don't have a WBCE on a public server (yet) where I could access it for a quick test. But in my patched WB classic sp4, if I revert the admin/settings/save.php (i.e. remove the additional calls to $database->escapeString), the problem goes away.

mrbaseman

Core-Team

Re: General settings / website footer

...if I revert the admin/settings/save.php (i.e. remove the additional calls to $database->escapeString), the problem goes away.

Attention: The one around $setting_name should stay for security reasons. Only the call of escapeString for $value is too much. Alternatively, one could ommit the add_slashes a few lines before

florian

Administrator

Re: General settings / website footer

Oh _there_ in the options > website header/footer you mean, I did not test that, sorry.
(Personally I think its maybe not so good that in these fields HTML is allowed, if you need HTML information on all pages in the Header/Footer, this should be placed in the template.)
My suggestion for a solution: I would not revert the changes of the security patch but place the needed/wanted information in the template.

---

Wir Benötigen: Cents, Euros... jetzt spenden!

norhei

Developer

Re: General settings / website footer

You are right , thats the Problem.

Questionis how to change. The $admin->add_slashes($value) i s a special one , it only adds slashes if magick_quote hasn't done this already.

$database->escapeString($value, ";") always does this.

hmmm... maybe thats a solution:

Admin/settings/save.php around line 191

[== PHP ==]
         if (!in_array($value, $disallow_in_fields) && (isset($_POST[$setting_name]) || $passed == true)) {
            $value = trim(strip_magic($value));
            $sql = 'UPDATE `' . TABLE_PREFIX . 'settings` '
            . 'SET `value`=\'' . $database->escapeString($value, ";") . '\' '
            . 'WHERE `name`!=\'wb_version\' AND `name`=\'' . $database->escapeString($setting_name) . '\'';
            if (!$database->query($sql)) {
                $admin->print_error($database->get_error, $js_back);
                break;
            }
        }
    }
}
function strip_magic($input) {
    if (get_magic_quotes_gpc() and is_string($input)) {
            return stripslashes($input);
        }
        return $input;
}

As long as we will allow PHP < 5.4  there is a possibility that magic quotes is on.

What do you think ?

For 1.2.x i add the function to Class WB like add_slashes and stip_slashes.
When we go for PHP >= 5.4  we finally remove this.

mrbaseman

Core-Team

Re: General settings / website footer

My suggestion for a solution: I would not revert the changes of the security patch

That's why I emphasized that the escaping of the $setting_name should stay, but $value is already secured and therefore the security patch did not add (much) more security but introduced a regression of features working properly. WB classic is also fighting with unwanted changes of the behavior due to these modifications in sake of security.

You are right , thats the Problem.
Questionis how to change. The $admin->add_slashes($value) i s a special one , it only adds slashes if magick_quote hasn't done this already.
[...]
As long as we will allow PHP < 5.4  there is a possibility that magic quotes is on.  What do you think ?

what about much the simpler approach:

[== PHP ==]
         if (!in_array($value, $disallow_in_fields) && (isset($_POST[$setting_name]) || $passed == true)) {
            $sql = 'UPDATE `' . TABLE_PREFIX . 'settings` '
            . 'SET `value`=\'' . $database->escapeString($value) . '\' '
            . 'WHERE `name`!=\'wb_version\' AND `name`=\'' . $database->escapeString($setting_name) . '\'';
            if (!$database->query($sql)) {
                $admin->print_error($database->get_error, $js_back);
                break;
            }
        }
    }
}

btw. escapeString takes one parameter only. There was an additional ";" which doesn't make sense to me. Or did I overlook something?

but place the needed/wanted information in the template.

that's a different approach: just change a couple of lines above:

$allow_tags_in_fields = array();

... and the problem would disappear, too. But that's not only a security patch then. That's a change of features. But adding a link in the footer (e.g. a copyright remark or a link to the Impressum page) make sense in my opinion. Of course lengthy html formattings belong into the template, but a few tags can indeed be handy here.

albatros

Mitglied

Re: General settings / website footer

Wow, you are so fast! Thanks a lot so far!
I now, that I could use the template for the footer. Hardcoding the footer would be ok for me. But I posted instead, because I didn't know if this problem would only appear at this point or maybe anywhere else.

mrbaseman

Core-Team

Re: General settings / website footer

That's why I emphasized that the escaping of the $setting_name should stay, ...

correcting myself: Unless there is already a malicious $setting_name in the database, we don't even have to escape $setting_name, because that variable is not posted. It already comes out of the database:

    while ($setting = $res_settings->fetchRow()) {
        $old_settings[$setting['name']] = $setting['value'];
        $setting_name = $setting['name']; 

and

Questionis how to change. The $admin->add_slashes($value) i s a special one , it only adds slashes if magick_quote hasn't done this already.

$database->escapeString($value, ";") always does this.

so, magick_quote might have done the job already. Then, maybe we should better stay with the add_slashes - i.e. revert this save.php file to the original one (before the security fix), because $value is escaped by add_slashes if needed and setting_name doesn't ever need any escaping?

Beitrag geändert von mrbaseman (02.03.2016 14:20:37)

norhei

Developer

Re: General settings / website footer

1. I would prefer of having HTML in Header and Footer as many old instalations rely on this and upgrading would be a really ugly lot of work.

2. Right now i am reworking the whole Settings /Preferences Stuff, so in 1.2.x this hopefullly is no longer an issue.

3. The main issue where the 2 completely unprotected Vars in PReferences FE and BE at lease thats what i could see fom WB Classic code.

4. 

correcting myself: Unless there is already a malicious $setting_name in the database, we don't even have to escape $setting_name, because that variable is not posted. It already comes out of the database:

Thats right. We can savely remove this .
There might be a few other useless changes, even if i already removed quite a few. It was a real ugly job extracting what was wrong from multiple undocumented releases , where someone hat plastered   $database->escapeString() everywhere :-( But as long as they do no harm i leave em till i rewrite the section anyway.

5. Did someone else tested the patch i posted ?
If it's functional i think  it's  not beautifull  but will do its job  till this section is reworked anyway.

6. Yes ";" semicolon is used in quite a few injections . Marmot posted a nice link in the other thread which was deleted by accident. So just use Google on "SQL injection semicolon" .

Ill be back in a few hours

mrbaseman

Core-Team

Re: General settings / website footer

BTW: in the currently publicly available sp6 rc3.1 the sql update query looks like this:

            $value = trim($database->escapeString($value));
            $sql = 'UPDATE `'.TABLE_PREFIX.'settings` SET '
                 . '`value`=\''.$value.'\' '
                 . 'WHERE  `name`=\''.$database->escapeString($setting_name).'\'';

so, add_slashes has been replaced by escapeString. And for the $setting_name, well... it doesn't hurt to escape that one, but it's probably not needed

mrbaseman

Core-Team

Re: General settings / website footer

that's the escapeString method of the database object.

    public function escapeString($unescaped_string)
    {
        return mysqli_real_escape_string($this->db_handle, $unescaped_string);
    }

In my opinion it just discards any additional parameter after $unescaped_string. Or is there any magic of passing through additional parameters inside php upwards until any nested function call picks it up from the stack? I doubt, but if there is, it's definitely no good coding style when you rely on such a mechanism ;-)

norhei

Developer

Re: General settings / website footer

Was in the wrong framework ...:-(

In my old forks escapeString() had an additional parameter for extra chars to escape ...
MAking a new patch tomorrow morning .  As Martin found a  few additional places where validation could be made better by simply casting to Integer.

I am not sure if double escaping like in RC3.1 is a good idea?  in addition if Magic Quotes is on , this will triple escape leading to ////// chains .

Therefore i tried to remove magic quotes and then single escape whith escapeString()

norhei

Developer

Re: General settings / website footer

Ok next version of patch.

Some additional patches , removed some non functional code.  (e.g. , ";")
Removed /admin/interface.php , because it caused trouble whith WB Classic .
This is no problem as its not relevant for the functionality of the patch. 
And hopefully helps WB Classic users not to upgrade to an unstable version.

Please test it before we add it to the download section.

Comment:
(v4) is because we had quite a few internal version steps, and versions had to be redone a few times, sorry.

florian

Administrator

Re: General settings / website footer

Made a quick test, seems to work

Do I have to update all my 120 Websites again?

---

Wir Benötigen: Cents, Euros... jetzt spenden!

norhei

Developer

Re: General settings / website footer

If you don't need the Header/Footer functionality, i would say no.
All other security patches are really minor ones that only possibly could be exploitet.

mrbaseman

Core-Team

Re: General settings / website footer

Do I have to update all my 120 Websites again?

but it would be a nice large scale test...

florian

Administrator

Re: General settings / website footer

Well, the weatherforecast for the weekend is ugly, and before I'm bored I could do the same stuff like last weekend again...
Great luck that I'm at least ready with adding a "Datenschutzerklärung" on all these pages. So I think uploading the few files is done rather fast.

---

Wir Benötigen: Cents, Euros... jetzt spenden!