New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Too much escaping in radclient #2299

Open
alandekok opened this Issue Sep 10, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@alandekok
Member

alandekok commented Sep 10, 2018

Issue type

too much escaping in radclient

  • Defect - Unexpected behaviour (obvious or verified by project member).

Defect

How to reproduce the issue

Put this into a file:

User-Name = "bob"
User-Password = "bob"
Filter-Id := "bob\\\t"

then run ./radclient -xxxd ./share/ -f file -x localhost auth testing123

You will see:

 Data:		01  05  62 6f 62
		02  12  24 68 80 1f 4a b1 51 d8 2f f4 bf fb bd 90 d0 65
		0b  07  62 6f 62 5c 09

i.e. three backslashes followed by t have been converted to a literal tab (0x09)

The issue seems to be that fr_pair_list_afrom_str() calls fr_pair_raw_from_str()which calls gettoken() to parse the double-quoted string. gettoken() does un-escaping.

Once fr_pair_raw_from_str() returns, fr_pair_list_afrom_str() then calls fr_pair_make(), which calls fr_pair_value_from_str(), which then calls value_data_from_str(), which also does un-escaping.

The issue seems to be that fr_pair_raw_from_str() is returning raw.quote == T_SINGLE_QUOTED_STRING, because there's no xlat expansion in it.

That's a pretty bad hack.

There are two solutions:

  1. Instead of calling gettoken(), just parse / return the double quoted string, without doing un-escaping

  2. instead of calling fr_pair_make() for these strings, just call fr_pair_strcpy():

I suspect that (2) is correct. It's what's done for xlat'd strings already.

The issue likely matters elsewhere, e.g. the "users" file. I'm wary of changing existing behavior. But this is arguably an annoying bug, and should be fixed. Also, few people should have backslash / escape sequences in attribute values.

alandekok added a commit that referenced this issue Sep 11, 2018

@alandekok

This comment has been minimized.

Show comment
Hide comment
@alandekok

alandekok Sep 11, 2018

Member

I've pushed a (mostly) fix for the v4 branch. But there's something odd going on. It seems that a direct call to fr_pair_make() results in much un-escaping, but calling the functions directly is fine.

We still have to track down the root cause of the problem, and fix it.

Member

alandekok commented Sep 11, 2018

I've pushed a (mostly) fix for the v4 branch. But there's something odd going on. It seems that a direct call to fr_pair_make() results in much un-escaping, but calling the functions directly is fine.

We still have to track down the root cause of the problem, and fix it.

@arr2036

This comment has been minimized.

Show comment
Hide comment
@arr2036

arr2036 Sep 12, 2018

Member

I vote 2. The fix for v4 looks ok. I mean pair_make() will be removed at some point soon, I already removed the vast majority of calls to it, it's just still used int he users file code.

Member

arr2036 commented Sep 12, 2018

I vote 2. The fix for v4 looks ok. I mean pair_make() will be removed at some point soon, I already removed the vast majority of calls to it, it's just still used int he users file code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment