Fenix: [Bug] WebPush notification event data filled with null bytes

[Jordandev678]

Steps to reproduce

Send web push notification through Firefox

The same server side code is used to send the same string in all tests, looping through endpoints and sending them with web-push-php. I've confirmed this code successfully pushes events to Chrome mobile, Chome desktop and Firefox Desktop. This image shows the console view in Firefox Desktop.

However the data received by the Firefox Mobile client is garbled. However the 'correct' data does appear to be at the end although this obviously breaks the.json() call. The image below is the mobile client connected via USB debugging.

I can confirm the device in question was successfully receiving these notifications prior to updating. I can also confirm after having another user on a different device try Firefox the notifications did not work for them either.

Expected behavior

Push data is received on the client as it was sent

Actual behavior

Push data is present but is preceeded by lots of null bytes.

Device information

• Android device: Nokia 6
• Fenix version: 79

[Jordandev678]

After further debugging I have narrowed it down to the padding in the encryption. Disabling the padding results in push notifications coming through again. At a guess I'd say you're just decrypting the notifications and passing it straight on without also checking if the encryption method supported padding and the message has some that needs removed.

Note that in many libraries such as the one in use here - padding the message is the default behavior for increased security.

[jonalmeida]

@jrconlin I don't think this is something we support right now, is that correct?

[jrconlin]

This may be a bug in the underlying crypto library. IIUC, RFC 8291, padding separation is determined by a 0x02 octet, and any data between it and the last 16 octets must be discarded. I am unable to verify if the UA code is not observing this, but I can try looking into it next week.

On a somewhat side note, I'm not really sure that padding in this case gets you what you think it gets you, since any hostile party can easily determine the pad and discard it as well.

Still, this is not following the RFC, so we should address it.

[rfk]

The RFC9281 bits are the responsibility of https://github.com/mozilla/rust-ece/, please file a bug in that repo and we can triage this next week.

[rfk]

(Or at least, it sounds like a missing testcase that should be added to that repo, even if the underlying bug is in the rc_crypto backend linked above)

[Jordandev678]

I've already tracked this down in rust-ece and added it as a test case. It looks like an issue with aesgcm encoding. See https://github.com/mozilla/rust-ece/pull/45

[jrconlin]

Ah. We should absolutely check to make sure we're doing padding verification for aes128gcm, however if this bug is only for aesgcm, I would strongly recommend that we mark it as WONTFIX.

aesgcm is a Working Group legacy encryption protocol that we are providing some transition support for, however it is not the correct standard that should be used. At some point, Push providers may drop aesgcm the way that we've dropped the older formats already. Subscription providers should really use the RFC standard aes128gcm encoding format. It is my understanding that aes128gcm support is available with most encoding libraries, however some libraries may not have switched it to be the default yet. (I've reached out to a number of these authors, but admit I have not followed through to ensure that they've carried this out.)

[Jordandev678]

I understand that aesgcm is the old one and the goal is to replace it. However given it may be the default in some libraries still and is still in use by some providers either way it seems worth fixing.

Given I've got the fix potentially already sorted and ready to go in that pull request can that be merged in instead of just marking it won't fix? No point breaking stuff unnecessarily, this worked fine prior to fenix after all.

[tanguilp]

When sending notifications on the new Fenix version, a notification is shown but when clicking on it, Fenix opens but does not open a new tab with the given URL from the data field. Could it be related to this issue? @Jordandev678

[Jordandev678]

@tanguilp I wouldn't expect so. But it will break anything that expected the .json() call to work. So if you overrode the ServiceWorkerGlobalScope.onnotificationclick event with something that relied on .json() returning data and telling it what to do when clicked then it could cause it. But if not then it's a separate bug.

Incidentally, my fix has been merged into rust-ece so hopefully it'll work its way through the update system. I don't know if you guys want to keep this open until it does or not but that should hopefully about wrap this bug up.

[jonalmeida]

When sending notifications on the new Fenix version, a notification is shown but when clicking on it, Fenix opens but does not open a new tab with the given URL from the data field. Could it be related to this issue? @Jordandev678

@tanguilp could you file a separate issue for this if Jordan's changes do not fix the issue? Preferably with the link you sent if it isn't self-identifying.

[tanguilp]

Will do but I am using .json(), so could very well be related to this bug. Here's the full script:

self.addEventListener('push', function(event) {
  data = event.data.json();

  const promiseChain = self.registration.showNotification(data.title, data);

  event.waitUntil(promiseChain);

});

self.addEventListener('notificationclick', function(event) {
  clients.openWindow(event.notification.data.url);
}, false);

[Jordandev678]

In my case the .text() call returned the null bytes but the .json() call threw an error because of them. So if you were running into this issue I wouldn't expect the notification to show at all because you're not catching/recovering from an error thrown by that call. Not that there would be much of a point - not having the data is generally not a recoverable error after all.

So I expect it's a different problem. Assuming there isn't any other issues it looks like either Fenix isn't calling the notificationclick event or it's not responding to the openWindow request (as it's not already open perhaps?).

[TitanNano]

@Jordandev678 fenix is indeed currently not calling the notificationclick event. There is an open issue for that.

[jonalmeida]

Ah yes, as Jovan mentioned correctly that issue is tracked here: https://github.com/mozilla-mobile/android-components/issues/7477

[rfk]

if this bug is only for aesgcm, I would strongly recommend that we mark it as WONTFIX.

And of course, it transpires that our own FxA service is still using the legacy aesgcm encoding, ref https://github.com/mozilla/fxa/issues/6434 😅

(I don't think the library we use does any padding, so we probably weren't triggering this bug ourselves, but still, ugh...)

[jonalmeida]

With the ece changes merged, this should have made it into release by now. @Jordandev678 is there anything else left here?