dnsdist: SpoofRaw with comma creates malformed packets

[wojas]

• Program: dnsdist
• Issue type: Bug report

Short description

SpoofRaw creates malformed packets when TXT records include a comma.

Environment

Master as of yesterday (b67e520) and same on 1.6.0.

Steps to reproduce

newServer{address='8.8.8.8'}
function spoof_raw_txt(dq)
    local content = '\001,' -- the comma causes an issue
    return DNSAction.SpoofRaw, content
end
addAction(AllRule(), LuaAction(spoof_raw_txt))

The same happens when using the FFI interface.

Expected behaviour

dig -t txt returns a TXT record with only a comma.

Actual behaviour

;; Warning: Message parser reports malformed message packet.

; <<>> DiG 9.16.15 <<>> -p 7899 @localhost -t txt example.com
; (2 servers found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 46310
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0
;; WARNING: Message has 1 extra bytes at end

;; QUESTION SECTION:
;example.com.			IN	TXT

Other information

No other characters cause this issue:

newServer{address='8.8.8.8'}
function spoof_raw_txt(dq)
    local s = ''
    for i = 0, 255 do
        local ch = string.char(i)
        if ch ~= ',' then
            s = s .. ch
        end
    end
    local content = string.char(#s) .. s
    return DNSAction.SpoofRaw, content
end
addAction(AllRule(), LuaAction(spoof_raw_txt))

example.com.		60	IN	TXT	"\000\001\002\003\004\005\006\007\008\009\010\011\012\013\014\015\016\017\018\019\020\021\022\023\024\025\026\027\028\029\030\031 !\"#$%&'()*+-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\127\128\129\130\131\132\133\134\135\136\137\138\139\140\141\142\143\144\145\146\147\148\149\150\151\152\153\154\155\156\157\158\159\160\161\162\163\164\165\166\167\168\169\170\171\172\173\174\175\176\177\178\179\180\181\182\183\184\185\186\187\188\189\190\191\192\193\194\195\196\197\198\199\200\201\202\203\204\205\206\207\208\209\210\211\212\213\214\215\216\217\218\219\220\221\222\223\224\225\226\227\228\229\230\231\232\233\234\235\236\237\238\239\240\241\242\243\244\245\246\247\248\249\250\251\252\253\254\255"

[rgacogne]

Looks like we might need some form of escaping now that we handle multiple values separated by commas..

[Habbie]

Just to be sure, I checked, and indeed #10063 introduces this.

[Habbie]

Looks like we might need some form of escaping now that we handle multiple values separated by commas..

Or add support for returning tables?

[Habbie]

Or add support for returning tables?

Oh, not that easy, because of the DNSAction prototype.

[wojas]

One possible solution would be to add a SpoofRawMulti action where every record value would be prefixed by two length bytes. A convenience function could be provided to convert a table of strings to the right length-prefixed string.

This would be a bit ugly. We could hide the ugliness with a pure Lua wrapper around the callback that converts SpoofRaw, {...} into SpoofRawMulti, spoofRawMultiFromTable({...}).

[Habbie]

One possible solution would be to add a SpoofRawMulti action where every record value would be prefixed by two length bytes.

I had the exact same idea. Must be a good idea then!

[rgacogne]

Oh, not that easy, because of the DNSAction prototype.

I just tried to turn the return value of LuaAction from a std::tuple<int, boost::optional<string>> into std::tuple<int, boost::optional<boost::variant<string, std::vector<std::pair<int, std::string>>>>> but there is a 10% penalty for very small functions, so unfortunately that does not look like a viable option.

[rgacogne]

I'm pondering adding Lua bindings to store a table into the DNSQuestion object instead. We would then check if that table has been set when we receive a DNSAction.SpoofRaw result, and fall back to the string instead. Any thoughts?

[Habbie]

I think that makes sense. Sounds like it would keep the interface cheap -usually-, and not terribly hard to use even for the special cases. We should include some actual full examples in the docs if we do this, I think.

[wojas]

That would work fine for plain Lua. The FFI interface would need an alternative way to set this.

[rgacogne]

Sure, we would add a setter function in the FFI interface :)

[rgacogne]

It turns out that there is already a non-FFI way to do this via DNSQuestion.spoof(): https://dnsdist.org/reference/dq.html?#DNSQuestion:spoof
I'm going to add the necessary bits to have a FFI counterpart.

[mdavids]

Could this be related?

https://mailman.powerdns.com/pipermail/dnsdist/2022-December/001267.html

[rgacogne]

Could this be related?

https://mailman.powerdns.com/pipermail/dnsdist/2022-December/001267.html

Very likely, yes.