Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/charset.c:qp_encode() ensure that a multi-octet character is not split across adjacent 'encoded-word's #2663

Closed
dilyanpalauzov opened this issue Mar 10, 2019 · 11 comments
Assignees
Labels
3.0 affects 3.0 3.1 affects 3.1 dev releases bug

Comments

@dilyanpalauzov
Copy link
Contributor

dilyanpalauzov commented Mar 10, 2019

As required per RFC 2047 Section 5.

PATCH v2

@rsto rsto self-assigned this Mar 11, 2019
@rsto rsto added the bug label Mar 11, 2019
@rsto
Copy link
Member

rsto commented Mar 11, 2019

I can't reproduce the issue described in the patch, but that's not to say there isn't an issue with the current implementation.

E.g. running your test input on my local build of the master branch implementation of charset_encode_mimeheader returns:

=?UTF-8?Q?=D0=94=D0=B8=D0=BB=D1=8F=D0=BD_=D0=9F=D0=B0=D0=BB=D0=B0=D1=83=D0=B7?=
=?UTF-8?Q?=D0=BE=D0=B2?=

Note how it legally splits the encoded word only at valid UTF-8 boundaries. This is ensured by the ISUTF8CONTINUATION check. If the input wouldn't be a header, then splitting within UTF-8 words doesn't seem to be forbidden by RFC 2045.

What this issue shows is that the QP-encoder for headers can overrun the allowed line length. It should indeed split after the =D1=83 sequence close to the end of the first line.

I'm working on fixing the line length issue, and your suggested solution looks reasonable. But if you found a way to produce illegal UTF-8 word splits in Q-encoded headers in the current implementation, please let me know.

@dilyanpalauzov
Copy link
Contributor Author

I discovered this with cyrus-3.0.

@dilyanpalauzov
Copy link
Contributor Author

dilyanpalauzov commented Mar 11, 2019

The way the error was exposed was by uploading a meeting with

ORGANIZER;CN=Дилян Палаузов…
ATTENDEE:mailto:x@different-host

Cyrus generated email/iMIP invitation with From: using as display-name the Common-Name converted to UTF-8-Q-encoding. K9 had troubles displaying the display name, as a single character was spread over two encoded-words.

@rsto
Copy link
Member

rsto commented Mar 11, 2019

Thanks. @elliefm I've fixed this for the development branch, but the implementation on the 3.0 branch differs. Not sure how to handle this: backporting the fix shouldn't be much on an issue, or are we going to pull in the latest charset library on 3.0 anyway?

The development branch fix is 3ca31fe

@dilyanpalauzov
Copy link
Contributor Author

The macro ISUTF8CONTINUATION is not used anymore and can be deleted.

@dilyanpalauzov
Copy link
Contributor Author

Looking at cunit/charser.testc:test_encode_mimeheader():

There is no difference, if one inserts in a header =?UTF-8?Q?abc? or abc, as at the end the same text is displayed. Therefore the tests

TESTCASE("abc", "abc");
TESTCASE("abc\r\n", "=?UTF-8?Q?abc?=");

shall produce the same result – unencoded abc.

Likewise a test for "70times the letter A, space, some multi-octet characters” is missing. The result shall be two lines, the first line being 70 times the letter A, without =?UTF-8?Q?…

I do not understand the purpose of feeding \r\n in charset_encode_mimeheader(). E.g. in

TESTCASE("abc\r\n xyz", "=?UTF-8?Q?abc?=\r\n =?UTF-8?Q?xyz?=");

depending on the purpose the space could be encoded as underscore.

@elliefm elliefm self-assigned this Mar 12, 2019
@elliefm elliefm added 3.0 affects 3.0 3.1 affects 3.1 dev releases labels Mar 12, 2019
@elliefm
Copy link
Contributor

elliefm commented Mar 12, 2019

@rsto Generally I don't plan to add new dependencies for 3.0.x releases. So the way to go would be to backport just the fix to 3.0, rather than the whole new version.

I don't have a strong enough understanding of the charset handling to think about side effects/ramifications, but superficially at least, the fix looks sensible enough. :)

I'm planning to do a 3.0.9 release later this week (was meant to be late last week, but I got delayed). How comfortable are you with the (backported) fix? If you're happy that it's release quality, drop it onto the 3.0 branch ASAP and it'll get included. Otherwise, sit on it until after the release, then it can percolate in git for a while.

@rsto
Copy link
Member

rsto commented Mar 12, 2019

@elliefm I've pushed 29218f4 on the 3.0 branch

@karagian
Copy link

@dilyanpalauzov have you done similar CalDAV tests with shared calendars too? I really thought the functionality you describe in these issues was not fully implemented. I ran into an issue with an event containing an ORGANIZER property when stored on a shared calendar and reported it as #2655
I'm really interested in knowing if this functionality has been implemented with shared calendars in mind... sorry in advance if this comment is out of topic here...

@dilyanpalauzov
Copy link
Contributor Author

#2655 is unrelated. Adding calender objects with attendee/organizer to a shared calender is not defined.

@brong brong unassigned rsto Jun 7, 2019
@elliefm
Copy link
Contributor

elliefm commented Oct 11, 2019

this was fixed in 3.0.9

@elliefm elliefm closed this as completed Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 affects 3.0 3.1 affects 3.1 dev releases bug
Projects
None yet
Development

No branches or pull requests

4 participants