From mboxrd@z Thu Jan  1 00:00:00 1970
From: Adolf Belka <adolf.belka@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] connscheduler.cgi: Remove cleanhtml command from Remark
Date: Thu, 07 Mar 2024 12:18:37 +0100
Message-ID: <ce3f381a-e25d-4783-a28e-7bd127a65753@ipfire.org>
In-Reply-To: <55e772fb-9252-438a-a7a3-af634ac16426@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============9191236903971748405=="
List-Id: <development.lists.ipfire.org>

--===============9191236903971748405==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Michael,

I think I know how to solve the problem.

I tested out using HTML::Entities::encode_entities in a very simple Perl=20
program and found I got the same type of entity encoding as in the WUI=20
CGI pages.

However, if I treated the string of characters as utf8 then the=20
HTML::Entities::encode_entities gave the results expected.

So I need to figure out how to treat the remark strings as utf8 and=20
hopefully that should fix the problem. At least I have a view of a path=20
forward on this issue now, that will keep the protection of the=20
cleanhtml command while also allowing characters with diacritical marks,=20
plus special characters such as the Cyrillic alphabet and also things=20
like the german eszet that currently all get mangled.

Will let you know how I get on.

Additionally I will also later on create patches for the WUI CGI pages=20
for the Firewall Groups and for WIO as they do not use the cleanhtml=20
command at all yet they also have many Remark entries. I will also check=20
out the other WUI pages that don't use the cleanhtml command to see if=20
they have remarks etc that should use it.

Regards,

Adolf.

On 06/03/2024 23:23, Adolf Belka wrote:
> Hi Michael,
>
> On 06/03/2024 22:28, Michael Tremer wrote:
>> Hello Adolf,
>>
>> I believe that I cannot merge these patches.
> Then you need to also look back at the dns.cgi patch for the bug fix=20
> due to german umlauts being changed. The acceptance of that patch is=20
> what made me create these patches as they all had the same problem=20
> with remarks as well. If this can't be accepted as is then that patch=20
> needs to be reverted.
>
> https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dcommit;h=3D7c6ff5ff12331a53f=
416080a44c8d6145e78bfac=20
>
>>
>> The reason simply is that it would create a store cross-site=20
>> scripting attack vector because someone could store some <script>=20
>> tags with some JS which will be executed if another admin opens the=20
>> same page.
>>
>> That is why we escape the content so that if there are any special=20
>> characters like <> for HTML tags they won=E2=80=99t be interpreted by the =

>> browser.
>>
>> We might have problems where we accidentally call the cleanhtml=20
>> function twice which should show garbage. We might also have a=20
>> problem where the function is not giving us the output that we want.
>>
>> Which strings have been causing problems? Just German umlauts like=20
>> =E2=80=9C=C3=A4=C3=B6=C3=BC=E2=80=9D?
> It is any character that has an accent or other diacritical mark.
>
> I just tried entering the following into the remark section for a dns=20
> server entry as an example
> =C3=84 =C3=A3 =C3=B6 =C3=A2 =C3=A1 =C3=A0
>
> and the remark was changed to
> =C3=83=E2=80=9E =C3=83=C2=A3 =C3=83=C2=B6 =C3=83=C2=A2 =C3=83=C2=A1 =C3=83
>
> and if I edit the entry but don't change the remark the new characters=20
> above get changed again into
> =C3=83=C6=92=C3=A2=E2=82=AC=C5=BE =C3=83=C6=92=C3=82=C2=A3 =C3=83=C6=92=C3=
=82=C2=B6 =C3=83=C6=92=C3=82=C2=A2 =C3=83=C6=92=C3=82=C2=A1 =C3=83=C6=92=C3=82
>
>
> I would have expected that running cleanhtml should result in=20
> characters that are considered safe after one run through but it seems=20
> that the encoding creates characters that are encoded again by=20
> cleanhtml as being unsafe and then those ones are again still=20
> considered unsafe.
>
> If the cleanhtml command needs to stay being used also for the remark=20
> entries then I have no idea how to allow german umlauts and other=20
> accented characters to be shown correctly because they are all higher=20
> bit ascii characters and those are encoded by default by the cleanhtml=20
> process as being considered unsafe so I would either need some help on=20
> how to deal with it or maybe someone else needs to pick up the=20
> original bug#12395
>
> The only thing I found is that cleanhtml calls the escape function=20
> which calls the HTML::Entities::encode_entities command but that=20
> command can have an additional option which defines the characters=20
> that are considered unsafe, but then I would need some guidance on=20
> which characters are considered unsafe and if that set applies to all=20
> invocations of the cleanhtml command. ie should a modified cleanhtml=20
> command with the extra option for the HTML::Entities::encode_entities=20
> command only be used for some of the cleanhtml calls and if so would=20
> that be only the remark/comment entries?
>
> Regards,
> Adolf.
>
>>
>> -Michael
>>
>>> On 5 Mar 2024, at 19:44, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>
>>> - Using cleanhtml on Remarks means that all characters with=20
>>> diacritical marks such as
>>> =C2=A0=C2=A0 umlauts or grave accents etc get encoded into other characte=
rs.
>>> - If Freifunk M=C3=BCnchen e.V. is entered as a remark it gets converted =
to
>>> =C2=A0=C2=A0 Freifunk M=C3=83=C2=BCnchen e.V.
>>> - cleanhtml is only removed from Remarks or Comment fields. In other=20
>>> places it has been
>>> =C2=A0=C2=A0 left in place.
>>>
>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> ---
>>> html/cgi-bin/connscheduler.cgi | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/html/cgi-bin/connscheduler.cgi=20
>>> b/html/cgi-bin/connscheduler.cgi
>>> index cc78cbc1b..817247cc4 100644
>>> --- a/html/cgi-bin/connscheduler.cgi
>>> +++ b/html/cgi-bin/connscheduler.cgi
>>> @@ -2,7 +2,7 @@
>>> #########################################################################=
######=20
>>>
>>> # #
>>> # IPFire.org - A linux based=20
>>> firewall=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0 #
>>> -# Copyright (C) 2007=C2=A0 Michael Tremer & Christian=20
>>> Schmidt=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #
>>> +# Copyright (C) 2007-2024=C2=A0 IPFire Team=20
>>> <info(a)ipfire.org>=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #
>>> # #
>>> # This program is free software: you can redistribute it and/or=20
>>> modify=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #
>>> # it under the terms of the GNU General Public License as published=20
>>> by=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #
>>> @@ -186,7 +186,7 @@ if ( ($cgiparams{'ACTION'} eq 'add') ||=20
>>> ($cgiparams{'ACTION'} eq 'update') )
>>> =C2=A0=C2=A0 $CONNSCHED::config[$i]{'DAYSTYPE'} =3D=20
>>> lc($cgiparams{'ACTION_DAYSTYPE'});
>>> =C2=A0=C2=A0 $CONNSCHED::config[$i]{'DAYS'} =3D $l_days;
>>> =C2=A0=C2=A0 $CONNSCHED::config[$i]{'WEEKDAYS'} =3D $l_weekdays;
>>> -=C2=A0 $CONNSCHED::config[$i]{'COMMENT'} =3D=20
>>> &Header::cleanhtml($cgiparams{'ACTION_COMMENT'});
>>> +=C2=A0 $CONNSCHED::config[$i]{'COMMENT'} =3D $cgiparams{'ACTION_COMMENT'=
};
>>>
>>> =C2=A0=C2=A0 &CONNSCHED::WriteConfig;
>>> }
>>> --=20
>>> 2.44.0
>>>
>>
>

--=20
Sent from my laptop


--===============9191236903971748405==--