Allow username-affixed token to be used with INFO_AUTOLOGON flag set in client

[lcniel]

This PR simply implements the suggestion by @matt335672 given in #2392 (comment) based on the fix provided by @galeksandrp in #2392 to the token-affixed-to-username scheme originally introduced in #1668. The PR seems to have been abandoned by the original poster, so I figure I might as well open a new one. Of course if @galeksandrp would prefer to follow up on their original PR instead I will be happy to close this one.

This will allow the "token-based" login to work with clients such as the current version of mstsc.exe and remmina, which don't necessarily expose the INFO_AUTOLOGON flag for easy configuration.

Fundamentally, just like when the feature was originally added, this provides a cross-platform means by which users can log on using short-lived compact tokens (which may simply contain something like a securely encrypted timestamp and username) so that a web service can authenticate a user using e.g. pam-jwt or a similar tool.

I have manually tested with Remmina on a Fedora 43 client and a Rocky 8 server. Will test with mstsc.exe ASAP. ETA: Can verify that it also works with mstsc.exe 10.0.26100.1 (WinBuild.160101.0800).

[lcniel]

I have now also tested it with a Guacamole web client as well as with the Mac OS Windows App.

[matt335672]

Thanks for this @lcniel

One comment - following the code through, the only place where the RDP_AUTO_LOGON flag seems to get checked is in the routine you're modifying. The original code only sets the password member variable in the client_info struct if this flag is set. If the password field is specified, and RDP_AUTO_LOGON isn't set (unlikely I know) the original code won't use the specified password, and your code will.

So I think this code in your PR:

    if (ts_info_utf16_in(s, len_password, self->rdp_layer->client_info.password, sizeof(self->rdp_layer->client_info.password)) != 0)
    {
        LOG(LOG_LEVEL_ERROR, "ERROR reading password");
        return 1;
    }

Needs to be something like:

    if (flags & RDP_LOGON_AUTO)
    {
        if (ts_info_utf16_in(s, len_password, self->rdp_layer->client_info.password, sizeof(self->rdp_layer->client_info.password)) != 0)
        {
            LOG(LOG_LEVEL_ERROR, "ERROR reading password");
            return 1;
        }
    }
    else
    {
        // Skip the password
        if (!s_check_rem_and_log(s, len_password + 2, "Parsing [MS-RDPBCGR] TS_INFO_PACKET Password"))
        {
            return 1;
        }
        in_uint8s(s, len_password + 2);
    }

The rest of the code is the same.

Thoughts?

[lcniel]

Thanks for this @lcniel

One comment - following the code through, the only place where the RDP_AUTO_LOGON flag seems to get checked is in the routine you're modifying. The original code only sets the password member variable in the client_info struct if this flag is set. If the password field is specified, and RDP_AUTO_LOGON isn't set (unlikely I know) the original code won't use the specified password, and your code will.

So I think this code in your PR:

    if (ts_info_utf16_in(s, len_password, self->rdp_layer->client_info.password, sizeof(self->rdp_layer->client_info.password)) != 0)
    {
        LOG(LOG_LEVEL_ERROR, "ERROR reading password");
        return 1;
    }

Needs to be something like:

    if (flags & RDP_LOGON_AUTO)
    {
        if (ts_info_utf16_in(s, len_password, self->rdp_layer->client_info.password, sizeof(self->rdp_layer->client_info.password)) != 0)
        {
            LOG(LOG_LEVEL_ERROR, "ERROR reading password");
            return 1;
        }
    }
    else
    {
        // Skip the password
        if (!s_check_rem_and_log(s, len_password + 2, "Parsing [MS-RDPBCGR] TS_INFO_PACKET Password"))
        {
            return 1;
        }
        in_uint8s(s, len_password + 2);
    }

The rest of the code is the same.

Thoughts?

I think you're right, I pushed this change but I will need a minute to manually test it.

I also think if I am not misreading that 575-583 and 620-627 are rendundant? I don't see how it can make it to 620-627 and then be caught by that guard unless I am missing some total edge case.

575-583

 // If we require credentials, don't continue if they're not provided
    if (self->rdp_layer->client_info.require_credentials)
    {
        if ((flags & RDP_LOGON_AUTO) == 0)
        {
            LOG(LOG_LEVEL_ERROR, "Server is configured to require that the "
                "client enable auto logon with credentials, but the client did "
                "not request auto logon.");
            return 1;
        }
        if (len_user == 0 || len_password == 0)
        {
            LOG(LOG_LEVEL_ERROR, "Server is configured to require that the "
                "client enable auto logon with credentials, but the client did "
                "not supply both a username and password.");
            return 1;
        }
    }

620-627

    else if (self->rdp_layer->client_info.require_credentials &&
             !(flags & RDP_LOGON_AUTO))
    {
        LOG(LOG_LEVEL_ERROR, "Server is configured to require that the "
            "client enable auto logon with credentials, but the client did "
            "not request auto logon.");
        return 1; /* credentials on cmd line is mandatory */
    }

[matt335672]

Yes you're right. 620-627 can be removed.

[lcniel]

Yes you're right. 620-627 can be removed.

Done, squashed, and also tested on my part. I don't know if I have a client that can test different cases without RDP_AUTO_LOGON set though, I think that's like a compile-time variable in FreeRDP...

[lcniel]

@matt335672 I did a rebase now (there was a minor conflict since the flag changed name). How do you feel about merging this?

[matt335672]

Hi @lcniel

Yes - I'll merge this. Sorry about the delay. Like the rest of the open source world we've been triaging a lot of security advisories recently.

[lcniel]

Hi @lcniel

Yes - I'll merge this. Sorry about the delay. Like the rest of the open source world we've been triaging a lot of security advisories recently.

No need to apologize at all and thanks for all the hard work.