Subscriptions-transport-ws: [SECURITY] Bypassing the onConnect server handler

[mattkrick]

Couldn't find a way to privately report this, but it isn't too critical & folks probably use proper auth on their schema, right? :sweat_smile:
Per the docs, the onConnect server handler can be used as a form of gatekeeping if you use it to throw or returns false.
This can be bypassed by simply not sending the INIT message, which means the initPromise will simply be true. Then, the client can send a GQL_START message, even if the onConnect was designed to block it.

This can be fixed by ensuring the INIT was received, but that's dirty.
Instead, I propose deprecating the INIT message & using the native 'connection' event on the websocket server. any connectionParams from the client can then be attached to the url & parsed like url.parse(upgradeReq.url) by the server. that way, errors are caught on connection, not on message & it eliminates the limbo that exists after connection & before init.

[AlpacaGoesCrazy]

Just tested this myself v0.9.16 works like described: change client to not pass init message and bypass onConnect authorization. How is this not fixed for so long? Even official docs suggest to use this method.
Maybe we can set connectionContext.initPromise to resolve false until we get INIT message if onConnect is set to fix this?
@abernix Can we get some attention to this issue?

[mattkrick]

@AlpacaGoesCrazy that would introduce a mem leak, which would be an easy attack vector for a DOS attack

[OmgImAlexis]

Is there a currently a fix that can be applied manually?

[mattkrick]

@OmgImAlexis unfortunately the problem is the protocol, not the implementation. if security is important, i'd recommending rolling your own. In my case, it meant better security & connectivity, reduced throughput, and ability to use the socket for more than just graphql.

Here's what I've been using in prod for the last couple years:

• for connectivity (reconnect logic, offline message queue, websocket fallbacks): https://github.com/mattkrick/trebuchet-client/blob/master/src/SocketTrebuchet.ts#L22
• for graphql messages: https://github.com/mattkrick/graphql-trebuchet-client/blob/master/src/GQLTrebuchetClient.ts#L71
• sample server handler: https://github.com/ParabolInc/action/blob/master/packages/server/wsHandler.ts

[kbobrowski]

@AlpacaGoesCrazy resolving it to false would still allow to start subscription, this condition is checked only on CONNECTION_INIT.

This seems like a bug, START message sent by the client will result in ERROR if client sent CONNECTION_INIT before and is not authenticated, but will result in DATA if no CONNECTION_INIT has been sent. Not sure if this was intended behavior?

Perhaps allowing START only when CONNECTION_INIT has been received as suggested by @mattkrick would be the simplest solution, and I don't think it's more dirty than current behavior of overriding initPromise.

At the very minimum docs should be updated to better document current behavior, I created simple repo which documents various ways of interacting with current implementation: https://github.com/kbobrowski/apollo-subscription-exploit

[OmgImAlexis]

Seems requiring a directive to pass an apiKey prevents this from causing harm as the connection never gets passed an asyncIterator.

For those wondering the context.user is setup by the initPromise meaning if they skip it we still don't grant them access instead they get an error. Technically I think we could even go as far as dropping the connection but this seems "okay".

const createSubscription = channel => {
    return {
        subscribe(_: unknown, __: unknown, context: any) {
            if (!context.user) {
                throw new AppError('<ws> No user found in context.', 500);
            }

            // Check the user has permission to subscribe to this endpoint
            ensurePermission(context.user, channel);

            // If the user has an invalid/missing context.user
            // it never gets to sub to the iterator
            return pubsub.asyncIterator(channel);
        }
    };
};

[kbobrowski]

Yes this is the additional step which has to be taken with current implementation to ensure that authentication is working. The problem is that user may have the impression that it's enough to define onConnect and Apollo server will take care of authentication (as it is currently documented)

[OmgImAlexis]

At least for now this should be enough for those waiting for an fix to be implemented.

[OmgImAlexis]

The solution suggested by @OmgImAlexis only works if your ensurePermission works as synchronous function.

In many cases checking user permission requires database queries and it's an async behavior, thus can't live inside subscribe resolver as it doesn't accept promises.

There's still a way to use async permission checkers before the subscription made, but you need to tweak the async iterator returned by pubsub implementation. Check out this repo if anyone interested.

What do you mean? Yes they do. I'm using this right now with promises.