Calendar encoding problem

[kartagis]

Hi,

I've just installed OwnCloud, and I would like to report a problem.

I create a contact with the character Ö (Muzaffer Tolga Özses), and I set their birthday. However, it looks like " Muzaffer Tolga Özses's birthday" in the calendar. I checked the database in the oc_contacts_cards and verified it looks fine.

[kartagis]

Hi,

I forgot to click on the label 'bug' prior to filing this issue, and I was told I couldn't do it afterwards. So, could you do it for me and move this issue to the 'bug' queue?

Thanks,

[tanghus]

Duplicate of issue #207

[kartagis]

Hi,

Issue #207 hardly represents my issue. It is only related to ' (apostrophe), while my issue is that umlaut and such can't be properly viewed.

[tanghus]

The bug is the same. That user just didn't have any special characters in the name.
I saw the other report first, even though you reported it before him. My apologies, but the important thing is that we get the bug fixed.

[tanghus]

@schiesbn As far as I can see this is caused by 672b924
First OCP\Util::sanitizeHTML() encodes HTML to entities, then OCP\JSON::encodedPrint() encodes the resulting entities.
I fail to see the reason for not stripping tags before saving it to the db. IMHO that is the best practice: Clean before save, encode in presentation layer if needed.

[tanghus]

OTOH it's probably this one owncloud/core@ce66759 @LukasReschke

[LukasReschke]

@tanghus This was a vendor fix for fullcalendar 1.5.4 (http://arshaw.com/fullcalendar/)

[RandolfCarter]

After upgrade to 4.5.2 from 4.5.1, this also happens for me - umlauts (ö.ä,ü,Ö,Ä,Ü) and ' showing as their entity in the calendar web interface.

[tanghus]

@LukasReschke I realize that, but then we should make sure that titles are unescaped in calendar. Hence my question above: Why not strip tags on save?

[LukasReschke]

but then we should make sure that titles are unescaped in calendar.

👍

Why not strip tags on save?

I dislike this idea. There are maybe people who use < and > in titles. If we forbid these characters, we should display a warning if a user tries to use "<" and ">" there.
This is bad UX IMHO.

[tanghus]

Then we should have an OCP\Util::SanitizeInput() method that as a first step strips script tags. Often sanitizeHTML() breaks more than it fixes.

btw, this:

strip_tags("2 is < than 3 but > 1");

Produces the correct

"2 is < than 3 but > 1"

While

strip_tags("2 is <than 3 but> 1");

Produces

"2 is1"

Which is of course not good, but a corner case ;) IMO strip_tags() is better than nothing.

[Korodny]

Sorry about the duplicate (#502 was me), I was searching existing entries using labels, guess I should have used the search function only.

I'd like to stress that this bug only affects monthly view, all other views (including text bubbles and edit event) don't show this behaviour, at least not for umlauts or apostrophes.

[tanghus]

Well the bug is still open, and to solve it we need to remove sanizeHTML() from

apps/calendar/ajax/events.php

Line 34 in 672b924

OCP\JSON::encodedPrint(OCP\Util::sanitizeHTML($output));

which leaves it wide open for XSS, so I suggest adding a method something like this (not tested and I don't do regex, so it's cut'n'paste):

    /**
     * @brief Public function to sanitize input data
     *
     * This function is used to sanitize input data and should be applied on any
     * string or array of strings before saving it to data store.
     *
     * @param string or array of strings
     * @return array with sanitized strings or a single sanitized string, depends on the input parameter.
     */
    public static function sanitizeInput( &$value ) {
        $search = array(
                    '@<script[^>]*?>.*?</script>@si',  // Strip out javascript
                    '@<style[^>]*?>.*?</style>@siU',    // Strip style tags properly
        );

        if (is_array($value) || is_object($value)) {
            array_walk_recursive($value, 'OC_Util::sanitizeInput');
        } else {
            $value = trim(preg_replace($search, '', $value); 
        }
        return $value;
    }

And we should probably have a similar javascript method.

Edit: and this doesn't take care of onclick etc btw.

[LukasReschke]

Edit: and this doesn't take care of onclick etc btw.

https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet

As I don't have much time currently, I'll have a look later this week for a proper solution...

[tanghus]

That would be cool. You're more experiences in those matters :-)

On Monday 19 November 2012 09:53 Lukas Reschke wrote:

As I don't have much time currently, I'll have a look later this week for a

proper solution...

Med venlig hilsen / Best Regards

Thomas Tanghus

[kartagis]

I've just confirmed that it indeed only happens in monthly view.

Regards,

On 19 November 2012 20:58, Thomas Tanghus notifications@github.com wrote:

That would be cool. You're more experiences in those matters :-)

On Monday 19 November 2012 09:53 Lukas Reschke wrote:

As I don't have much time currently, I'll have a look later this week
for a

proper solution...

Med venlig hilsen / Best Regards

Thomas Tanghus

—
Reply to this email directly or view it on GitHubhttps://github.com/owncloud/apps/issues/205#issuecomment-10525984.

[RandolfCarter]

remove sanizeHTML() from

apps/calendar/ajax/events.php

Line 34 in 672b924

OCP\JSON::encodedPrint(OCP\Util::sanitizeHTML($output));

which leaves it wide open for XSS,

Hm, maybe I'm missing some point here, but why would removing one of the two duplicate encodings leave anything open for XSS? Shouldn't a single encoding to entities be enough to take care of preventing any HTML injection?

[tanghus]

You're right @RandolfCarter
I took an extra look at the js code, and plugging a few other holes was enough. I didn't mean to close this before I get some extra dev eyes on it though.
Still, I think we need a method for stripping potential XSS before saving it. That's The Right Way (TM) IMHO ;)