Re: [PATCH] [#48/T22] Added missing server reply on auth-agent-req

[Jakub Jelen <jjelen@xxxxxxxxxx>]

Hi,
thank you for the patch. Can you submit it as a merge request in gitlab?

https://gitlab.com/libssh/libssh-mirror/-/merge_requests

On 5/7/21 12:29 AM, Jeremy Cross wrote:
> [#48/T22] Added missing server reply on auth-agent-req when a
>   reply was requested by the client. PuTTY for Windows chokes without this
>   reply if "Allow agent forwarding" is enabled. Reply will be successful if
>   channel_auth_agent_req_function callback is defined. Based on an unmerged
>   patch by Jon Simons.
>
> Signed-off-by: Jeremy Cross <jcross@xxxxxxxxxxxxxxx>
> --
> diff --git a/src/channels.c b/src/channels.c
> index 4eea885b..663dbcaf 100644
> --- a/src/channels.c
> +++ b/src/channels.c
> @@ -692,7 +692,7 @@ SSH_PACKET_CALLBACK(channel_rcv_close) {
>   SSH_PACKET_CALLBACK(channel_rcv_request) {
>   	ssh_channel channel;
>   	char *request=NULL;
> -    uint8_t status;
> +    uint8_t want_reply;
>       int rc;
>   	(void)user;
>   	(void)type;
> @@ -705,7 +705,7 @@ SSH_PACKET_CALLBACK(channel_rcv_request) {
>   
>   	rc = ssh_buffer_unpack(packet, "sb",
>   	        &request,
> -	        &status);
> +	        &want_reply);
>   	if (rc != SSH_OK) {
>   		SSH_LOG(SSH_LOG_PACKET, "Invalid MSG_CHANNEL_REQUEST");
>   		return SSH_PACKET_USED;
> @@ -815,11 +815,29 @@ SSH_PACKET_CALLBACK(channel_rcv_request) {
>     if (strcmp(request, "auth-agent-req@xxxxxxxxxxx") == 0) {
>       SAFE_FREE(request);
>       SSH_LOG(SSH_LOG_PROTOCOL, "Received an auth-agent-req request");
> -    ssh_callbacks_execute_list(channel->callbacks,
> -                               ssh_channel_callbacks,
> -                               channel_auth_agent_req_function,
> -                               channel->session,
> -                               channel);
> +
> +    int status = SSH2_MSG_CHANNEL_FAILURE;

Please, keep the variables declaration in the beginning of blocks. I 
think it is not explicitly stated anywhere, but some older C standards 
and compiler implementation have issues with this.

> +    ssh_callbacks_iterate(channel->callbacks, ssh_channel_callbacks,
> +                    channel_auth_agent_req_function){
> +        ssh_callbacks_iterate_exec(channel_auth_agent_req_function,
> +                                   channel->session,
> +                                   channel);
> +        /* in lieu of a return value, if the callback exists it's supported */
> +        status = SSH2_MSG_CHANNEL_SUCCESS;
> +        break;
> +    }
> +    ssh_callbacks_iterate_end();
> +
> +    if (want_reply) {
> +        rc = ssh_buffer_pack(session->out_buffer,
> +                             "bd",
> +                             status,
> +                             channel->remote_channel);
> +        if (rc != SSH_OK) {
> +            return SSH_PACKET_USED;
> +        }
> +        ssh_packet_send(session);
> +    }
>   
>       return SSH_PACKET_USED;
>     }
> @@ -828,7 +846,7 @@ SSH_PACKET_CALLBACK(channel_rcv_request) {
>   	 * client requests. That means we need to create a ssh message to be passed
>   	 * to the user code handling ssh messages
>   	 */
> -	ssh_message_handle_channel_request(session,channel,packet,request,status);
> +	ssh_message_handle_channel_request(session,channel,packet,request,want_reply);
>   #else
>       SSH_LOG(SSH_LOG_WARNING, "Unhandled channel request %s", request);
>   #endif

Otherwise it looks fine.

Thanks,
--
Jakub Jelen
Senior Software Engineer
Crypto Team, Security Engineering
Red Hat, Inc.