I'm going to request a freeze exception for this. Would be good to have a review first.

(In reply to Michael Catanzaro from comment #0)
> Discovered by Kamil Paril

Kamil Paral

Created attachment 311681 [details] [review]
Don't crash on escaped null characters

Disallow escaped slashes as well.

Created attachment 311682 [details] [review]
Don't crash on escaped null characters

Disallow escaped slashes as well.

Created attachment 311683 [details] [review]
Don't crash on escaped null characters

Disallow escaped slashes as well.

Maybe I should just never post patches in the evenings, if it takes me this many tries. :)

Created attachment 311685 [details] [review]
Don't crash on escaped null characters

Disallow escaped slashes as well.

Review of attachment 311685 [details] [review]:

The patch looks good to me, feel free to push before 3.18 if you get the break approval.

::: embed/ephy-web-view.c
@@ +2535,3 @@
 
+  if (address) {
+    decoded_address = ephy_uri_safe_unescape (address);

Why not making ephy_uri_safe_unescape() NULL safe like g_uri_unescape_string()?

::: lib/ephy-uri-helpers.c
@@ +256,3 @@
+
+  // Security implications....
+  g_assert (uri_string);

What's the problem of returning NULL here instead of asserting?

I am somewhat leaning towards changing the assert g_return_val_if_fail and returning an empty string; that way, if such an issue ever does occur, we still get criticals but we don't crash.

(In reply to Carlos Garcia Campos from comment #7)
> ::: embed/ephy-web-view.c
> @@ +2535,3 @@
>  
> +  if (address) {
> +    decoded_address = ephy_uri_safe_unescape (address);
> 
> Why not making ephy_uri_safe_unescape() NULL safe like
> g_uri_unescape_string()?

I had trouble deciding whether to do that or not. My rationale for not being NULL safe was that it should probably never be called with NULL, and you're probably walking into some sort of crash if it is, so best crash early and harmlessly with g_assert. It's an attempt to turn future issues into harmless crashes.

But another approach would be to return an empty string instead. That would be safer in the sense that we won't ever crash, but it would almost always be hiding a bug, since we'd normally wind up with an empty string displayed somewhere in the UI. Hence I didn't pick that option.

But it turns out, where it is called here is actually a strange exception to the rule, where it's perfectly fine to have a NULL address. This call will need to be guarded with a conditional regardless, or just use g_uri_unescape_string directly instead of ephy_uri_safe_unescape.

> ::: lib/ephy-uri-helpers.c
> @@ +256,3 @@
> +
> +  // Security implications....
> +  g_assert (uri_string);
> 
> What's the problem of returning NULL here instead of asserting?

The point of the function is simply to assure that g_uri_unescape_string() never returns NULL. The code after it is called almost always assumes the result is not NULL. My mistake was passing a guaranteed non-NULL string into a function that could change it to NULL, and not changing the code to expect that.

(In reply to Michael Catanzaro from comment #8)
> I am somewhat leaning towards changing the assert g_return_val_if_fail and
> returning an empty string; that way, if such an issue ever does occur, we
> still get criticals but we don't crash.

I don't like using g_return macros for private functions, i usually limit it to check public values passed from users.

> (In reply to Carlos Garcia Campos from comment #7)
> > ::: embed/ephy-web-view.c
> > @@ +2535,3 @@
> >  
> > +  if (address) {
> > +    decoded_address = ephy_uri_safe_unescape (address);
> > 
> > Why not making ephy_uri_safe_unescape() NULL safe like
> > g_uri_unescape_string()?
> 
> I had trouble deciding whether to do that or not. My rationale for not being
> NULL safe was that it should probably never be called with NULL, and you're
> probably walking into some sort of crash if it is, so best crash early and
> harmlessly with g_assert. It's an attempt to turn future issues into
> harmless crashes.

g_uri_unescape_string() is null safe, and this is a convenient wrapper so I would keep it compatible.

> But another approach would be to return an empty string instead. That would
> be safer in the sense that we won't ever crash, but it would almost always
> be hiding a bug, since we'd normally wind up with an empty string displayed
> somewhere in the UI. Hence I didn't pick that option.

Never liked empty strings either, maybe we don't crash but it will be harder to debug it that happens, I prefer a crash :-)

> But it turns out, where it is called here is actually a strange exception to
> the rule, where it's perfectly fine to have a NULL address. This call will
> need to be guarded with a conditional regardless, or just use
> g_uri_unescape_string directly instead of ephy_uri_safe_unescape.

ephy_embed_utils_link_message_parse() is also null safe, the code is simpler without the if (address)

> > ::: lib/ephy-uri-helpers.c
> > @@ +256,3 @@
> > +
> > +  // Security implications....
> > +  g_assert (uri_string);
> > 
> > What's the problem of returning NULL here instead of asserting?
> 
> The point of the function is simply to assure that g_uri_unescape_string()
> never returns NULL. The code after it is called almost always assumes the
> result is not NULL. My mistake was passing a guaranteed non-NULL string into
> a function that could change it to NULL, and not changing the code to expect
> that.

(In reply to Carlos Garcia Campos from comment #9)
> I don't like using g_return macros for private functions, i usually limit it
> to check public values passed from users.

That's usually how I use them too... let's change it to a g_assert in master after we branch then. I got cold feet about adding a new assertion on release day.

> g_uri_unescape_string() is null safe, and this is a convenient wrapper so I
> would keep it compatible.

But since the purpose of the wrapper is to never return null, it would have to return an empty string if it gets passed null. I don't think that would ever be correct, and like you mentioned it would be harder to debug, so the function can't be null-safe.
 
> ephy_embed_utils_link_message_parse() is also null safe, the code is simpler
> without the if (address)

You're right. In this case, it would have been better to use plain g_uri_unescape_string(). ephy_uri_safe_unescape() is only useful for code that can't handle null. So I didn't need to change anything here after all. I will revert the changes in ephy_web_view_set_link_message() after the 3.18.0 release.

I'll post follow-ups in bug #755365.

(In reply to Michael Catanzaro from comment #10)
> You're right. In this case, it would have been better to use plain
> g_uri_unescape_string(). ephy_uri_safe_unescape() is only useful for code
> that can't handle null. So I didn't need to change anything here after all.
> I will revert the changes in ephy_web_view_set_link_message() after the
> 3.18.0 release.

Well no, I'm wrong, because the safe_unescape function still takes care to return the original string in case there's a slash. I think the current code is good enough; it's worse in the case that the following code is null-safe, but it's better otherwise.

Created attachment 311789 [details] [review]
Avoid using g_return_val_if_fail in ephy_uri_safe_unescape

Created attachment 311790 [details] [review]
Remove redundant call to g_task_set_check_cancellable

It's the default

I meant to put those patches in bug #755365